Bug 4834 - SPEC - JobExecution handed to JobOperator client "by reference" or "by value"?
SPEC - JobExecution handed to JobOperator client "by reference" or "by value"?
Status: NEW
Product: jbatch
Classification: Unclassified
Component: SPEC
1
PC Windows
: P5 normal
: ---
Assigned To: cvignola
future
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-24 19:02 UTC by ScottKurz
Modified: 2014-02-13 13:12 UTC (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description ScottKurz 2013-03-24 19:02:24 UTC
If I do this for a running job:		

  JobExecution exec = jo.getJobExecution(execId);

Do I see, in this 'exec' object, the updates as the job executes (e.g. BatchStatus getting set)? 

Or am I only getting a snapshot, and might I need to get a new one to see the ongoing execution?  

I.e. does the container have a reference (PBR) back to this JobExecution or is it taking a snapshot of the "value" (PBV)?

Since the spec has no remotable use cases... you might think it should maintain a reference.  

I don't think the TCK requires PBR behavior.. I think we ask for a new object each time we want to check the status whether we need to or not.

Since there are no setters on JobExecution, JobInstance, it seems PBR would have no undesirable side effects...

I think it should be spec'd and not left to the implementation, since it is a very basic aspect of the JobOperator programming model.
Comment 1 mminella 2013-03-24 19:53:29 UTC
The JobExecution should be viewed as a value object with no guarantees of it's ties to a running job.  jo.getJobExecution(execId) may return a previously run JobExecution as an example.   Also, with regards to the "no remoteable use cases", while the spec does not address remote executions, it does not preclude them either (there is nothing to prevent a user from developing a RemoteJobOperator implementation of the JobOperator interface for example).  

I don't think we can say within the spec that the JobExecution returned by the JobOperator is the same instance being acted upon by an executing job.
Comment 2 ScottKurz 2013-03-26 02:22:56 UTC
(In reply to comment #1)
Michael,

I agree with your angle on this one, which then leads me to the question of should the spec say that is is by value, e.g. that

  JobExecution exec = jo.getJobExecution(execId);
  JobExecution exec2 = jo.getJobExecution(execId);

return different instances?

Since there's no setters maybe it doesn't matter. 

Te RI uses "by-reference" for a currently-executed job and "by-value" for a terminated job.

The TCK assumes "by-value", e.g. in the polling routine we get a new JobExecution each time we poll rather than rechecking status on the existing.   But it doesn't require "by-value" either (obviously since the RI doesn't always use it).

I guess it could be left as an implementation detail.   The only code I can conjure up where it matters woul be:

// JobOperator "monitor"

while (..running..) {
  JobExecution exec = jo.getJobExecution(execId);
  logMonitorEntry(exec)
}

// now go process logged executions

-------------------

If I have PBR then instead of a list of "snapshots" I'm going to get a bunch of copies of the execution in its final state.   

Specifying PBV "breaks" the RI though...however, we could adjust still I believe.   Maybe it would be a clearer API to just specify "by value".
Comment 3 cvignola 2013-08-28 15:07:46 UTC
Leaving it open that a spec clarification might be desirable.
Comment 4 BrentDouglas 2013-10-28 07:51:31 UTC
I'm not sure if this is settled but can I put in a vote for the spec and TCK to be changed to allow JobExecution, StepExecution && JobInstance to be value classes.

The way I see it these values are tied to the the implementation of the job repository and requiring these classes to provide an up to date view of the repository will add unnecessary performance penalties to some implementations.
Comment 5 ScottKurz 2013-11-04 22:26:56 UTC
This hasn't been settled, so thanks Brent too for your comments.

I had another thought in this area: that the spec might say that  StepExecution#getPersistentUserData() is returned by value.

The way the RI is currently coded, it's theoretically possible for an operator to introduce side effects to the persistent user data that then affect say a Decider operating on the StepExecution.

We might also consider waiting to see if it turns out to be a real-world issue, since it might not be too likely someone would do that.
Comment 6 cf126330 2013-11-04 22:59:19 UTC
(In reply to ScottKurz from comment #5)
> This hasn't been settled, so thanks Brent too for your comments.
> 
> I had another thought in this area: that the spec might say that 
> StepExecution#getPersistentUserData() is returned by value.
> 
> The way the RI is currently coded, it's theoretically possible for an
> operator to introduce side effects to the persistent user data that then
> affect say a Decider operating on the StepExecution.
> 
> We might also consider waiting to see if it turns out to be a real-world
> issue, since it might not be too likely someone would do that.

By exposing only java.io.Serializable to the client, the expectation is the client should not modify it. 

I think it's more likely application itself (which knows the actual type of the user data) will use getPersistentUserData() to pass data between differnet parts, and the apps may expect the returned object is by reference.  StepContext.getPersistentUserData() and StepExecution.getPersistentUserData should be consistent in this regard.
Comment 7 ScottKurz 2013-11-04 23:41:58 UTC
(In reply to cf126330 from comment #6)
> StepContext.getPersistentUserData() and
> StepExecution.getPersistentUserData should be consistent in this regard.

Cheng, I agree with everything up until this last sentence, but wasn't sure where you were trying to end up.

You seemed to think that StepContext returns the persistent data by-reference and StepExecution returns it by-value, but then what did you mean in saying they should be "consistent"?
Comment 8 cf126330 2013-11-05 02:39:40 UTC
(In reply to ScottKurz from comment #7)
> (In reply to cf126330 from comment #6)
> > StepContext.getPersistentUserData() and
> > StepExecution.getPersistentUserData should be consistent in this regard.
> 
> Cheng, I agree with everything up until this last sentence, but wasn't sure
> where you were trying to end up.
> 
> You seemed to think that StepContext returns the persistent data
> by-reference and StepExecution returns it by-value, but then what did you
> mean in saying they should be "consistent"?

Application can access persistent user data via StepContext, and a job client can access persistent user data via StepExecution. So whether we settle on by-reference or by-value, the same rule should apply to both StepContext.getPersistentUserData() and StepExecution.getPersistentUserData(). I think by-reference is a better fit.  If an application is really concerned with the risk of persistent user data being modified by a job client, it can choose immutable or unmodifiable types for persistent data.
Comment 9 mminella 2013-11-05 03:39:59 UTC
I strongly think this should be by value (per my previous statements).  One thing to keep in mind is that the StepExecution and JobExecution may not retrieved within the same JVM as the job actually executing.  I could have a Job running in JVM 1 and use the JobOperator to retrieve the current state of the job from the job repository in JVM 2.  How would this work if it was pass by reference and the expectation was that modifications to the data by the client in JVM 2 was to impact JVM 1?  Allowing the client to modify the StepExecution/JobExecution in a way that the runtime is expected to react is a dangerous path IMHO.
Comment 10 ScottKurz 2014-02-13 13:12:32 UTC
Not committing to address in future, just listing as candidates and pointing out they're excluded from current Maintenance Rel.