Spring Batch
  1. Spring Batch
  2. BATCH-495

readers must not clear buffers on mark()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.0.rc1
    • Fix Version/s: 1.0.0
    • Component/s: Infrastructure
    • Labels:
      None

      Description

      Historically readers implemented TransactionSynchronization and the logic from afterCommit() was largely moved into current mark(). However, the contract of mark() differs significantly - it is not a point where the reader can safely forget the past events and clear its buffers. This relates to skipped item buffers in general and specifically to buffered XMLEvents in StaxEventItemReader.

      The basic issue is that mark() is called also before re-processing rolled back chunk.

        Activity

        Hide
        Robert Kasanicky added a comment -

        The big question is whether it is the readers that need to be patched or the execution logic - it doesn't make good sense to me that mark() is called immediately after reset() in rollback scenario. The execution logic is simply not smart enough to realize it rolled back to the already marked position and marks it again - forcing the readers to figure out the context.

        Show
        Robert Kasanicky added a comment - The big question is whether it is the readers that need to be patched or the execution logic - it doesn't make good sense to me that mark() is called immediately after reset() in rollback scenario. The execution logic is simply not smart enough to realize it rolled back to the already marked position and marks it again - forcing the readers to figure out the context.
        Hide
        Dave Syer added a comment -

        I think the extra call to itemHandler.mark() on line 290 of ItemOrientedStep is a hack to make one of the samples work. If I am right then it should be removed and the readers also patched to make them consistent?

        Show
        Dave Syer added a comment - I think the extra call to itemHandler.mark() on line 290 of ItemOrientedStep is a hack to make one of the samples work. If I am right then it should be removed and the readers also patched to make them consistent?
        Hide
        Lucas Ward added a comment -

        I think the problem is that 290 was put in for standard skip semantics of calling mark() before reading. If we move the mark on 290 up out of the repeatOperations.iterate, right after stream.open, we should be fine.

        Show
        Lucas Ward added a comment - I think the problem is that 290 was put in for standard skip semantics of calling mark() before reading. If we move the mark on 290 up out of the repeatOperations.iterate, right after stream.open, we should be fine.
        Hide
        Robert Kasanicky added a comment -

        I've moved the 290 mark() call out of the loop - this alone should be enough to fix the broken skip functionality discussed in http://forum.springframework.org/showthread.php?t=51388 (played a little with skipSample and it has the desired effect).

        Show
        Robert Kasanicky added a comment - I've moved the 290 mark() call out of the loop - this alone should be enough to fix the broken skip functionality discussed in http://forum.springframework.org/showthread.php?t=51388 (played a little with skipSample and it has the desired effect).
        Hide
        Robert Kasanicky added a comment -

        So, to wrap things up, do you think it is ok for readers to treat mark() as the point where they can forget the past events? I think yes, but we should state that in javadoc clearly.

        Show
        Robert Kasanicky added a comment - So, to wrap things up, do you think it is ok for readers to treat mark() as the point where they can forget the past events? I think yes, but we should state that in javadoc clearly.
        Hide
        Dave Syer added a comment -

        I think so too, as long by "past events" as we mean "past events as far back as the last mark", of course. We still have to be able to restore after a restart.

        Show
        Dave Syer added a comment - I think so too, as long by "past events" as we mean "past events as far back as the last mark", of course. We still have to be able to restore after a restart.
        Hide
        Robert Kasanicky added a comment -

        ok, agreed. I'll update the mark() javadoc and make FlatFileItemReader clear the skipped items buffer as other readers already do.

        Show
        Robert Kasanicky added a comment - ok, agreed. I'll update the mark() javadoc and make FlatFileItemReader clear the skipped items buffer as other readers already do.
        Hide
        Robert Kasanicky added a comment -

        Summary: readers can and should clear buffers on mark

        • updated mark() javadoc to state mark() is called only before processing new chunk, not before reprocessing
        • FlatFileItemReader modified to clear the skipped items buffer on mark (it is used in skipSample so its crucial that this reader behaves consistently with others)
        Show
        Robert Kasanicky added a comment - Summary: readers can and should clear buffers on mark updated mark() javadoc to state mark() is called only before processing new chunk, not before reprocessing FlatFileItemReader modified to clear the skipped items buffer on mark (it is used in skipSample so its crucial that this reader behaves consistently with others)
        Hide
        Dave Syer added a comment -

        Assume closed as resolved and released

        Show
        Dave Syer added a comment - Assume closed as resolved and released

          People

          • Assignee:
            Robert Kasanicky
            Reporter:
            Robert Kasanicky
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: