Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.3, 4.0-ALPHA
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      I would like to propose a TwoPhaseCommit interface which declares the methods necessary to implement a 2-phase commit algorithm:

      • prepareCommit()
      • commit()
      • rollback()

      The prepare/commit ones have variants that take a (Map<String,String> commitData) following the ones we have in IndexWriter.

      In addition, a TwoPhaseCommitTool which implements a 2-phase commit amongst several TPCs.

      Having IndexWriter implement that interface will allow running the 2-phase commit algorithm on multiple IWs or IW + any other object that implements the interface.

      We should mark the interface @lucene.internal so as to not block ourselves in the future. This is pretty advanced stuff anyway.

      Will post a patch soon

      1. LUCENE-3193.patch
        17 kB
        Shai Erera
      2. LUCENE-3193.patch
        15 kB
        Shai Erera

        Activity

        Shai Erera created issue -
        Hide
        Shai Erera added a comment - - edited

        Patch introduces:

        • TwoPhaseCommit interface + test
        • TPCTool (inner to TPC interface) for running a 2-phase commit algorithm
        • TPCWrapper, for running TPCTool w/ commitData
        • Modify IW to impl TPC
        • Add CHANGES entry (under the 3.x section).

        TPCWrapper is just a convenience class for passing a different commitData along w/ every TPC. If you have a better name, I'm open to suggestions (maybe CommitDataWrapper?)

        Core tests pass.

        Show
        Shai Erera added a comment - - edited Patch introduces: TwoPhaseCommit interface + test TPCTool (inner to TPC interface) for running a 2-phase commit algorithm TPCWrapper, for running TPCTool w/ commitData Modify IW to impl TPC Add CHANGES entry (under the 3.x section). TPCWrapper is just a convenience class for passing a different commitData along w/ every TPC. If you have a better name, I'm open to suggestions (maybe CommitDataWrapper?) Core tests pass.
        Shai Erera made changes -
        Field Original Value New Value
        Attachment LUCENE-3193.patch [ 12482146 ]
        Hide
        Shai Erera added a comment -

        This is basically ready for commit. I'd appreciate a review of the definitions plus TPCTool impl. I will give it a couple of days before I commit. We can always fix issues later.

        Show
        Shai Erera added a comment - This is basically ready for commit. I'd appreciate a review of the definitions plus TPCTool impl. I will give it a couple of days before I commit. We can always fix issues later.
        Hide
        Michael McCandless added a comment -

        Patch looks great! It's a nice addition.

        TwoPhaseCommitTool.execute should be @experimental?

        This seems similar to LUCENE-3131 (XA transactions support). I don't
        really understand what it takes to be "XA capable", but I assume it
        means building the right wrapper around IW's existing transactional
        APIs (like this).

        Shouldn't TwoPhaseCommit be @lucene.experimental instead (not
        internal)? Ie, the idea is an app can also impl this interface, and
        then use execute to commit all resources.

        Javadocs should maybe state that on rollback the writer will have been
        closed? (rollback closes the writer)

        One possible issue: if exceptions are hit in .commit(), which ought to
        be rare since prepareCommit does all the "hard work", we go and
        .rollback() all resources, even those that had successfully .commit'd,
        right? But, in general, this is tricky for a resource to handle
        (rolling back when commit had succeeded). EG, IndexWriter won't do
        this correctly – rollback will be a no-op in this case since it rolls
        back to the last successful commit.

        This is actually correctable, but it requires some more work to
        properly wrap the IndexWriter.

        Show
        Michael McCandless added a comment - Patch looks great! It's a nice addition. TwoPhaseCommitTool.execute should be @experimental? This seems similar to LUCENE-3131 (XA transactions support). I don't really understand what it takes to be "XA capable", but I assume it means building the right wrapper around IW's existing transactional APIs (like this). Shouldn't TwoPhaseCommit be @lucene.experimental instead (not internal)? Ie, the idea is an app can also impl this interface, and then use execute to commit all resources. Javadocs should maybe state that on rollback the writer will have been closed? (rollback closes the writer) One possible issue: if exceptions are hit in .commit(), which ought to be rare since prepareCommit does all the "hard work", we go and .rollback() all resources, even those that had successfully .commit'd, right? But, in general, this is tricky for a resource to handle (rolling back when commit had succeeded). EG, IndexWriter won't do this correctly – rollback will be a no-op in this case since it rolls back to the last successful commit. This is actually correctable, but it requires some more work to properly wrap the IndexWriter.
        Hide
        Shai Erera added a comment -

        TwoPhaseCommitTool.execute should be @experimental?

        Since it is an inner class of TPC, and it is marked @experimental, I thought it is implied. But I will add it there too for clarity.

        Shouldn't TwoPhaseCommit be @lucene.experimental instead

        You're right. I'll fix !

        Javadocs should maybe state that on rollback the writer will have been closed?

        This is mentioned in IndexWriter.rollback(). I don't think we should mention IW in TPC javadocs?

        One possible issue: if exceptions are hit in .commit()

        You're right. And TPCTool.execute has a NOTE about it, saying that for some TPC implementations, calling rollback() after commit() was successfully executed may be a no-op. I put it on execute because I think it is more related to the tool than TPC.rollback() definition.

        Show
        Shai Erera added a comment - TwoPhaseCommitTool.execute should be @experimental? Since it is an inner class of TPC, and it is marked @experimental, I thought it is implied. But I will add it there too for clarity. Shouldn't TwoPhaseCommit be @lucene.experimental instead You're right. I'll fix ! Javadocs should maybe state that on rollback the writer will have been closed? This is mentioned in IndexWriter.rollback(). I don't think we should mention IW in TPC javadocs? One possible issue: if exceptions are hit in .commit() You're right. And TPCTool.execute has a NOTE about it, saying that for some TPC implementations, calling rollback() after commit() was successfully executed may be a no-op. I put it on execute because I think it is more related to the tool than TPC.rollback() definition.
        Hide
        Michael McCandless added a comment -

        This is mentioned in IndexWriter.rollback(). I don't think we should mention IW in TPC javadocs?

        Ahh, you're right. Good!

        And TPCTool.execute has a NOTE about it, saying that for some TPC implementations, calling rollback() after commit() was successfully executed may be a no-op.

        Ahh, I missed that – super.

        But: perhaps a different exception should be thrown if there is a problem in the commit() loop vs the prepareCommit() loop? This way caller can differentiate this case?

        Show
        Michael McCandless added a comment - This is mentioned in IndexWriter.rollback(). I don't think we should mention IW in TPC javadocs? Ahh, you're right. Good! And TPCTool.execute has a NOTE about it, saying that for some TPC implementations, calling rollback() after commit() was successfully executed may be a no-op. Ahh, I missed that – super. But: perhaps a different exception should be thrown if there is a problem in the commit() loop vs the prepareCommit() loop? This way caller can differentiate this case?
        Hide
        Shai Erera added a comment -

        But: perhaps a different exception should be thrown if there is a problem in the commit() loop vs the prepareCommit() loop? This way caller can differentiate this case?

        Good idea. So instead of throwing IOE, we'll throw (Prepare)CommitFailException which takes a Throwable 'cause' and TPC 'obj'. We can make the two sub-class IOE for convenience (in case someone does not want to differentiate the two, it's easier to handle both cases in one catch block).

        I'll add these and upload a new patch by tomorrow.

        Show
        Shai Erera added a comment - But: perhaps a different exception should be thrown if there is a problem in the commit() loop vs the prepareCommit() loop? This way caller can differentiate this case? Good idea. So instead of throwing IOE, we'll throw (Prepare)CommitFailException which takes a Throwable 'cause' and TPC 'obj'. We can make the two sub-class IOE for convenience (in case someone does not want to differentiate the two, it's easier to handle both cases in one catch block). I'll add these and upload a new patch by tomorrow.
        Hide
        Shai Erera added a comment -

        Patch addresses Mike's comments (thanks !). Additionally, I extracted TPCTool into its own class, which now encapsulates the new exceptions + wrapper. Marked it @experimental too.

        I think this is ready for commit.

        Show
        Shai Erera added a comment - Patch addresses Mike's comments (thanks !). Additionally, I extracted TPCTool into its own class, which now encapsulates the new exceptions + wrapper. Marked it @experimental too. I think this is ready for commit.
        Shai Erera made changes -
        Attachment LUCENE-3193.patch [ 12482318 ]
        Hide
        Michael McCandless added a comment -

        Looks great Shai!

        Show
        Michael McCandless added a comment - Looks great Shai!
        Hide
        Shai Erera added a comment -

        Committed revision 1135204 (trunk).
        Committed revision 1135215 (3x).

        Show
        Shai Erera added a comment - Committed revision 1135204 (trunk). Committed revision 1135215 (3x).
        Shai Erera made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Robert Muir added a comment -

        bulk close for 3.3

        Show
        Robert Muir added a comment - bulk close for 3.3
        Robert Muir made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Shai Erera
            Reporter:
            Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development