Flume
  1. Flume
  2. FLUME-1321

BasicTransactionSemantics should never throw from close()

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: v1.1.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Release Note:
      Transaction close() now logs error message instead of throwing exceptions.

      Description

      Currently, BasicTransactionSemantics can throw from close(). This means that exceptions will be clobbered in cases where unexpected exceptions are thrown, since close() is idiomatically called in a finally block.

      1. FLUME-1321.2.patch
        2 kB
        Roshan Naik
      2. FLUME-1321.patch
        2 kB
        Roshan Naik

        Issue Links

          Activity

          Hide
          Mike Percy added a comment -

          Well, to enforce it via API (like make doClose() abstract) I think it's a 2.0 change, but getting these semantics due to implementations adhering to some new API rules should be doable in 1.x I think...

          Show
          Mike Percy added a comment - Well, to enforce it via API (like make doClose() abstract) I think it's a 2.0 change, but getting these semantics due to implementations adhering to some new API rules should be doable in 1.x I think...
          Hide
          Roshan Naik added a comment -

          ofcourse. I dropped ownership as it became evident that this was a longer term (2.0) thing. Should i leave the Review open ?

          Show
          Roshan Naik added a comment - ofcourse. I dropped ownership as it became evident that this was a longer term (2.0) thing. Should i leave the Review open ?
          Hide
          Mike Percy added a comment -

          BTW Roshan didn't mean anything personal about cancelling the patch available, I am just trying to work towards making the Patch Available review queue sane since the review board dashboard has reached a state where it's pretty much unusable.

          Show
          Mike Percy added a comment - BTW Roshan didn't mean anything personal about cancelling the patch available, I am just trying to work towards making the Patch Available review queue sane since the review board dashboard has reached a state where it's pretty much unusable.
          Hide
          Mike Percy added a comment -

          My take on the summary from the RB discussion are as follows:

          Goal from this change: Make the client semantics for channel simpler (don't require people to catch Throwable).
          Desired idiom:

          txn.begin()
          try {
            // do stuff, puts, whatever
            ch.put(...);
            ch.commit();
          } catch (FlumeException ex) {
            ch.rollback();
          } finally {
            txn.close();
          }
          

          Action items:
          1. Document this desired behavior in Channel interface
          2. Fix affected channels (FC is one) such that a close() always does a rollback on an open transaction, but will never throw (only log)

          #2 could be broken into subtasks of this JIRA

          Show
          Mike Percy added a comment - My take on the summary from the RB discussion are as follows: Goal from this change: Make the client semantics for channel simpler (don't require people to catch Throwable). Desired idiom: txn.begin() try { // do stuff, puts, whatever ch.put(...); ch.commit(); } catch (FlumeException ex) { ch.rollback(); } finally { txn.close(); } Action items: 1. Document this desired behavior in Channel interface 2. Fix affected channels (FC is one) such that a close() always does a rollback on an open transaction, but will never throw (only log) #2 could be broken into subtasks of this JIRA
          Hide
          Mike Percy added a comment -

          Temporarily cancelling patch available based on RB feedback

          Show
          Mike Percy added a comment - Temporarily cancelling patch available based on RB feedback
          Hide
          Mike Percy added a comment -

          Thanks Hari, I commented on RB

          Show
          Mike Percy added a comment - Thanks Hari, I commented on RB
          Hide
          Hari Shreedharan added a comment -

          Hi Mike,

          There is a conversation happening on Reviewboard which discussed the same issue.

          Show
          Hari Shreedharan added a comment - Hi Mike, There is a conversation happening on Reviewboard which discussed the same issue.
          Hide
          Mike Percy added a comment -

          Roshan I was thinking about this one recently... one example of where we must rollback on close is File channel... if not then a replayed WAL would be different than the FlumeEventQueue because close() is a no-op in file channel

          In fact right now the semantics are a bit wacky and in order to support this properly we have to evaluate how the other channels need to change to ensure correctness after this change.

          Maybe part of it would be requiring channels to implement rollback on close themselves, not sure if there are benefits to doing that over just doing it in the base class.

          Show
          Mike Percy added a comment - Roshan I was thinking about this one recently... one example of where we must rollback on close is File channel... if not then a replayed WAL would be different than the FlumeEventQueue because close() is a no-op in file channel In fact right now the semantics are a bit wacky and in order to support this properly we have to evaluate how the other channels need to change to ensure correctness after this change. Maybe part of it would be requiring channels to implement rollback on close themselves, not sure if there are benefits to doing that over just doing it in the base class.
          Hide
          Roshan Naik added a comment -

          adding revised patch for reference based on suggestions in code review. Buts its breaking many tests. still need more investigation

          Show
          Roshan Naik added a comment - adding revised patch for reference based on suggestions in code review. Buts its breaking many tests. still need more investigation
          Hide
          Roshan Naik added a comment -

          Mike, If we implicitly rollback in close(), we wont get the desired no-throw semantics in close().
          I suspect it is not be a good idea to do that. I have a revised the patch w/o the implicit rollback.

          Show
          Roshan Naik added a comment - Mike, If we implicitly rollback in close(), we wont get the desired no-throw semantics in close(). I suspect it is not be a good idea to do that. I have a revised the patch w/o the implicit rollback.
          Hide
          Mike Percy added a comment -

          Not sure about these lines:

          + if( Thread.currentThread().getId() != initialThreadId )
          + LOGGER.error("close() called from different thread than getTransaction()!");
          +

          I think we gotta throw on that one. That is absolutely bad bad bad.

          These ones I agree with:

          + if( state.equals(State.NEW) || state.equals(State.COMPLETED) )
          + LOGGER.error("close() called when transaction is {} " +
          + "- you must either commit or rollback first", state);

          As long as we do an implicit rollback when it happens. Would be great to get this posted to reviewboard. Thanks Roshan!

          Show
          Mike Percy added a comment - Not sure about these lines: + if( Thread.currentThread().getId() != initialThreadId ) + LOGGER.error("close() called from different thread than getTransaction()!"); + I think we gotta throw on that one. That is absolutely bad bad bad. These ones I agree with: + if( state.equals(State.NEW) || state.equals(State.COMPLETED) ) + LOGGER.error("close() called when transaction is {} " + + "- you must either commit or rollback first", state); As long as we do an implicit rollback when it happens. Would be great to get this posted to reviewboard. Thanks Roshan!
          Hide
          Brock Noland added a comment -

          Hi guys,

          I agree 100% on this. Just a note, there was a long conversation about this here: https://reviews.apache.org/r/4655/ and this is a dup of FLUME-1089.

          Brock

          Show
          Brock Noland added a comment - Hi guys, I agree 100% on this. Just a note, there was a long conversation about this here: https://reviews.apache.org/r/4655/ and this is a dup of FLUME-1089 . Brock
          Hide
          Roshan Naik added a comment -

          re-attaching file, prev attempt had some issues.

          Show
          Roshan Naik added a comment - re-attaching file, prev attempt had some issues.
          Hide
          Roshan Naik added a comment -

          Yes. I see the motivation. Does this look right ?

          Show
          Roshan Naik added a comment - Yes. I see the motivation. Does this look right ?
          Hide
          Mike Percy added a comment -

          I'm saying we should stop throwing IllegalStateException from close(). It should just log a warning and roll back the transaction, in my opinion. Right now, if the finally block is reached by a thrown exception, while the transaction is still open, the first exception is obliterated and replaced with the IllegalStateException. That sucks because it's difficult/impossible to tell what went wrong in the first place.

          Show
          Mike Percy added a comment - I'm saying we should stop throwing IllegalStateException from close(). It should just log a warning and roll back the transaction, in my opinion. Right now, if the finally block is reached by a thrown exception, while the transaction is still open, the first exception is obliterated and replaced with the IllegalStateException. That sucks because it's difficult/impossible to tell what went wrong in the first place.
          Hide
          Roshan Naik added a comment -

          Mike.. can you elaborate.. i dont see any possibility (other than Runtime exceptions).

          Show
          Roshan Naik added a comment - Mike.. can you elaborate.. i dont see any possibility (other than Runtime exceptions).

            People

            • Assignee:
              Unassigned
              Reporter:
              Mike Percy
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development