Bug 4764

Summary: SPEC - ItemReader Javadoc Issues
Product: jbatch Reporter: mminella
Component: sourceAssignee: cvignola
Severity: minor CC: issues, ScottKurz
Priority: P5    
Version: 1   
Target Milestone: ---   
Hardware: All   
OS: All   

Description mminella 2013-03-11 14:46:36 UTC
1.  The class level javadoc states that this is a batch artifact that "reads from a stream of items".  Given that java already has the concept of streams and that concept is not applicable here, we may want to choose another way to describe this.  I would say that we just want to state that this is a batch artifact that is responsible for providing input for a chunk based step.
2.  The javadoc for the open is to use case specific.  The open method is not about repositioning the reader as much as it is about resetting the state of the reader, whatever that may mean for the given implementation.  Also, in SB, we always pass in an ExecutionContext (the Serializable here) regardless of if it's a start or restart so we would advocate that the Serializable always be passed in and leave it to the implementation to determine what to do with that based on start vs restart.
3.  As with the class level javadoc, the readItem implies a java stream concept here that is not 100% accurate.
4.  checkpointInfo states that it returns the current position of the reader.  In fact, this should return whatever state is required to be persisted (regardless if it's position based or not).
Comment 1 cvignola 2013-03-16 20:43:14 UTC
I will incorporate all comments except the one about passing an empty Serializable.  This value is user-defined, not spec defined, so empty is simply null. Update coming in PFD v1.7
Comment 2 mminella 2013-03-18 16:39:01 UTC

The reason I bring up the open is because I'd like to still pass in the ExecutionContext that Spring Batch currently uses as that serializable.  Correct me if I'm wrong but the spec does not require that it be user generated, only that it be serializable?  If that's the case, we would still be able to support the existing Spring Batch ItemStream style contracts with the new JSR interfaces.
Comment 3 ScottKurz 2013-03-18 17:01:22 UTC
Michael's interpretation is news to me...

So in this view, there's no contract between container and app that the value returned by checkpointInfo() is the one passed to open() on restart.

I'd hope the TCK would fail such an implementation... as our intentions reflected an understanding that the instance returned by checkpointInfo() would be passed to open()  (I mean a deserialized equivalent of course).

That seems radically different than our current understanding.
Comment 4 mminella 2013-03-18 17:13:52 UTC
After further thinking, that won't work because close and checkpointInfo do not accept a serializable so there is no way for that to work...We'll have to handle this internally.
Comment 5 cvignola 2013-03-20 02:34:49 UTC
Michael, I see what you mean now.  The SB and JSR container/application contract governing checkpoint behavior are simply different.  

The JSR uses the concept of a checkpoint token that originates in the reader and is persisted through the container and then returned to the reader upon restart - the read essentially owns the object the container is merely the steward.

SB in comparison, uses a persistent workarea that is owned an managed by the container, but exposed to the reader to store and retrieve values across start/restart.

Both models are valid.  They achieve the same ends.  Their orientation is different. 

At this point in the schedule, we will have to go with the model in the JSR. I will still update the javadoc to avoid the stream confusion.