--------------Original Comment History----------------------------
Comment from = cf126330 on 2015-10-28 19:58:07 +0000
JobOperator currently has getJobExecutions(JobInstance) method to get all job executions belonging to the jobInstance passed in as param. Notice the param type is JobInstance instead of jobInstanceId as long, whereas the rest of JobOperator getter methods take long.
Apart from being consistent, passing a long jobInstanceId is also more natural. JobInstance, being a simple interface with only 2 methods (getInstanceId and getJobName), doesn't offer much more value than a simple jobInstanceId.
In case of a remote client, sometimes all it has is just the job instance id, not a JobInstance. And JobOperator does not have method to return a JobInstance by job instance id (the one we do have is getJobInstance(jobExecutionId)).
Comment from = ScottKurz on 2016-03-16 16:13:44 +0000
So the history here is that the JobOperator API at one point presented a lot of opportunities for errors, given all the 'long' values in batch: job instance, job execution, step execution. (Often in unit tests these weren't caught for awhile as the various values were incremented in parallel.)
The decision was made to only use job execution id as an input parm across all method. We also mostly return the JobInstance, JobExecution objs rather than long values, (with the exception of start/restart).
So we avoid the previous bugs by instead using:
getJobExecutions(JobInstance)
So, I think that we have from a consistency standpoint we have a reasonable story.
But like you said, the problem is this assumes my entry point into the job repository is the job execution id. If you are starting from an instance id, it's going to be convoluted.
Comment from = cf126330 on 2016-03-23 17:23:46 +0000
A good descriptive method name should be able to convey the intent of the api, so we don't have to be limited by "job execution ids in params only" rule. I think the need for such API (getting JobInstance by its id) is pretty common and reasonable.
getJobInstanceByInstanceId seems a bit too long. Maybe getJobInstanceById(long jobInstanceId), or getStepExecutionById(long stepExecutionId) is descriptive enough?
Originally opened as bug 7368 by cf126330
--------------Original Comment History---------------------------- Comment from = cf126330 on 2015-10-28 19:58:07 +0000
JobOperator currently has getJobExecutions(JobInstance) method to get all job executions belonging to the jobInstance passed in as param. Notice the param type is JobInstance instead of jobInstanceId as long, whereas the rest of JobOperator getter methods take long.
Apart from being consistent, passing a long jobInstanceId is also more natural. JobInstance, being a simple interface with only 2 methods (getInstanceId and getJobName), doesn't offer much more value than a simple jobInstanceId.
In case of a remote client, sometimes all it has is just the job instance id, not a JobInstance. And JobOperator does not have method to return a JobInstance by job instance id (the one we do have is getJobInstance(jobExecutionId)).
Comment from = ScottKurz on 2016-03-16 16:13:44 +0000
So the history here is that the JobOperator API at one point presented a lot of opportunities for errors, given all the 'long' values in batch: job instance, job execution, step execution. (Often in unit tests these weren't caught for awhile as the various values were incremented in parallel.)
The decision was made to only use job execution id as an input parm across all method. We also mostly return the JobInstance, JobExecution objs rather than long values, (with the exception of start/restart).
So we avoid the previous bugs by instead using:
So, I think that we have from a consistency standpoint we have a reasonable story.
But like you said, the problem is this assumes my entry point into the job repository is the job execution id. If you are starting from an instance id, it's going to be convoluted.
Obviously we can't just add:
JobInstance getJobInstance(long jobInstanceId)
it would need to be:
JobInstance getJobInstanceByInstanceId(long jobInstanceId)
Maybe also consider:
StepExecution getStepExecutionByStepExecutionId(long stepExecutionId)
Other suggestions?
Comment from = cf126330 on 2016-03-23 17:23:46 +0000
A good descriptive method name should be able to convey the intent of the api, so we don't have to be limited by "job execution ids in params only" rule. I think the need for such API (getting JobInstance by its id) is pretty common and reasonable.
getJobInstanceByInstanceId seems a bit too long. Maybe getJobInstanceById(long jobInstanceId), or getStepExecutionById(long stepExecutionId) is descriptive enough?