4 Replies Latest reply on Nov 4, 2009 8:26 AM by Clebert Suconic

    TransactionOperation.afterCommit

    Clebert Suconic Master

      IMO, TransactionOperation.afterCommit should never fail.

      I mean... at that point everything is already committed. If there is any chance of a fail it should happen before the prepare or commit. (That's actually written on the XA spec somwhere I guess).


      But it happens that afterCommit throws Exception on its sigature.

      And the only operation that can actually throw an exception is AddDuplicateIDOperation.

      So, we should remove throws Exception from TransactionOperation IMO, and have AddduplicateIDOperation refactored to not throw any exception on afterCommit.


      any thoughts?

        • 1. Re: TransactionOperation.afterCommit
          Clebert Suconic Master

          Or maybe.. that operation could just be ignored in case of failure (from DuplicateIDChannelImpl::addToCacheInMemory:


          private synchronized void addToCacheInMemory(final byte[] duplID, final long recordID) throws Exception
           {
           cache.add(new ByteArrayHolder(duplID));
          
           Pair<ByteArrayHolder, Long> id;
          
           if (pos < ids.size())
           {
           // Need fast array style access here -hence ArrayList typing
           id = ids.get(pos);
          
           cache.remove(id.a);
          
           // Record already exists - we delete the old one and add the new one
           // Note we can't use update since journal update doesn't let older records get
           // reclaimed
           id.a = new ByteArrayHolder(duplID);
          
           storageManager.deleteDuplicateID(id.b); <<< this is the only throwing exception
          
           id.b = recordID;
           }
           else
           {
           id = new Pair<ByteArrayHolder, Long>(new ByteArrayHolder(duplID), recordID);
          
           ids.add(id);
           }
          
           if (pos++ == cacheSize - 1)
           {
           pos = 0;
           }
           }
          


          • 3. Re: TransactionOperation.afterCommit
            Andy Taylor Master

            A few points, firstly there are actually 2 methods that throw an exception, RefsOperation is the other.

            Secondly, its still valid for commit to throw an exception i think, as long as the chances are minimal . Think of a database tx, all the work is done on prepare such as updating the tables, checking integrity constraints etc but the actual commit can still fail if the database connection goes down. It is then up to the Transaction Manager to decide what to do. In the case you show isnt the storageManager.deleteDuplicateID(id.b);
            method basically the commit. If we can handle this failing we can catch the exception and log it or alternatively throw a runtime exception if you want to remove the exception from the method signature.

            Regarding the second one, RefsOperation, this is far more complex. It calls postAcknowledge which removes the message from the journal if it is the last reference, again what should we do here, log a message.

            The other stuff that happens on refsOperation is to do with handling paging and handling large messages. Nearly every operation on both these all throw an exception. following the code down its mainly because of handling file resources etc. This exception handling probably needs refactoring but i'm not sure how best to change it as yet or if it should be left until post 2.0.

            thoughts?

            • 4. Re: TransactionOperation.afterCommit
              Clebert Suconic Master


              Yes.. the commit itself could fail, what is ok. The client would receive the Exception about the call not being successful what is ok.



              Regarding the second one, RefsOperation, this is far more complex. It calls postAcknowledge which removes the message from the journal if it is the last reference, again what should we do here, log a message.

              The other stuff that happens on refsOperation is to do with handling paging and handling large messages. Nearly every operation on both these all throw an exception. following the code down its mainly because of handling file resources etc. This exception handling probably needs refactoring but i'm not sure how best to change it as yet or if it should be left until post 2.0.

              thoughts?


              I'm just concerned that some of the transactional stuff was mistakenly moved to afterCommit when it should be on beforeCommit(). I wasn't 100% sure, but I had the impression the postACK done on the afterCommit would break transactionality.

              The IDCache could be safely ignored without a problem, the postACK is what was concerning me.