Review Board 1.7.22


BasicTransactionSemantics should avoid throw-ing from close()

Review Request #10362 - Created April 9, 2013 and updated

Roshan Naik
flume-1.4
FLUME-1321
Reviewers
Flume
flume-git
Loggin error instead of throwing exception in cases where close() is called w/o rollback or abort being called.

 
Total:
2
Open:
2
Resolved:
0
Dropped:
0
Status:
From:
Posted (April 9, 2013, 8:31 p.m.)

   

  
Is this what we want?  Can we have a test for this?
Posted (April 9, 2013, 8:46 p.m.)

   

  
I agree with Brock. This should not be the case. The file channel can lose data if we don't rollback (unless there is a replay)
  1. Yeah that is a good point. The file channel needs rollback to be called.... 
  2. Hari, Brock,
       Not clear to me what the implied suggestion for change is. Are you suggesting rollback() should be called from within close() ? Or ...  leave it as it originally was ?
  3. Hi,
    
    My first comment was that the code looks incorrect to me in that it prints an error message when the transaction is NEW or COMPLETED. Hari brings up a good point though. File channel requires a rollback otherwise data will be stuck in limbo. Memory channel may have this as well, not sure.  Therefore, I think if the transaction has not been rolledback/committed we need to execute a rollback.
  4. Ok. If we add an automatic rollback inside close() .. 
    
    
    - It will have a significant impact to the programming model for transactions. Essentially  there will be no need to call rollback() and then close() .. folks can directly call close and never bother calling rollback(). Today close() is only serving one purpose as far as i can tell.. perform some validations to ensure programmers are using the transactions correctly.
    
    - close() will  continue to throw and this jira's intent will not be addressed.
    
    It seems to me that this jira should be dropped.  Thoughts ?
  5. Yeah I think we'd have to call rollback, catching and logging any exception. This behavior would be similar to the db where you can close a connection without closing a statement and you can close a statement without closing a result set.
    
    I think we should hear from some other committers before making the change I describe above.
  6. It'd still be "polite" and recommended to rollback the transaction if not technically required. We could also log and error if we had to log rollback the transaction.
Posted (April 16, 2013, 10:51 p.m.)
I think it'd be great to preserve or enable the Lock-like idiom as a best-practice, i.e.

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

So whatever gets us there (right now we have to catch Throwable which is pretty horrible) I would support.

May require changes to the channel implementations which is fine. After mulling it over a bit, I think we should encourage channel implementations to do their own rollback-on-close and log an ERROR if the txn is still open at close time, because it will give them more ability to react to implementation specific details, like if rollback throws in the middle of a close.
  1. BTW, I meant to write txn.close() in the finally block above
  2. So ..  
    - BasicTransactionSemanitcs.doClose() will be marked abstract and all the channels will implement this in their transaction class 
    - BasicTransactionSemanitcs.close()  method's task will henceforth be delegated down to the channel's   doClose()  
    
    is that right ?
  3. I think we need to wait for Flume 2.0 to do that since it's an API break. Probably what we should do instead is simply document the API contract for Channel.close() implementations and fix any broken implementations in the core.