[GitHub] jkosh44 commented on issue #978: Use CompleteableFuture compose to centralize commit logic

[GitBox]

jkosh44 commented on issue #978: Use CompleteableFuture compose to centralize 
commit logic
URL: https://github.com/apache/fluo/issues/978#issuecomment-357830449
 
 
   Ok so I created a pull request, #1001. It's not passing every test but it 
only seems to be failing 3 tests.Those three test are all from `FailureIT` and 
it's because `TransactionImpl.commitPrimaryColumn(CommitData cd, Stamp 
commitStamp)` isn't returning false when it's supposed to. I'm pretty sure it's 
how I implemented exception handling, if a `CommitException` is thrown it's 
somehow not propagated back up to `commitPrimaryColumn`. I'm going to look at 
it some more but I though maybe a fresh set of eyes would be helpful.
   
   In general the way I dealt with the Visible For Testing methods were as 
follows.  I would create a first step as whatever intermediate step that 
testing method was starting at, fill in the rest of the steps using `andThen`, 
and then call compose. In the appropriate step classes themselves in 
`getMainOp` i would check if `stopAfterPrimaryCommit` or `stopAfterPreCommit` 
were true and if so I would return false. Then in `getFailureOp` if 
`stopAfterPrimaryCommit` or `stopAfter'PreCommit` were true I'd just call 
`cd.commitObserver.committed();` It's not super clean in that Essentially I'd 
be saying the operation failed if those test values were true even though 
technically they didn't fail. I wasn't sure of a cleaner solution but though 
this was a good first approach.
   
   Edit: right after typing this I just thought of a cleaner way. We wouldn't 
even need the two booleans if while filling in the next steps using `andThen` 
we stopped at the step we wanted to stop at. So for `commitPrimaryColumn` 
instead of 
   ```
   GetCommitStampStepTest firstStep = new GetCommitStampStepTest();
         firstStep.setTestStamp(commitStamp);
   
         firstStep.andThen(new WriteNotificationsStep()).andThen(new 
CommitPrimaryStep())
             .andThen(new DeleteLocksStep()).andThen(new FinishCommitStep());
   
         firstStep.compose(cd);
   ```
   we'd have 
   ```
   GetCommitStampStepTest firstStep = new GetCommitStampStepTest();
         firstStep.setTestStamp(commitStamp);
   
         firstStep.andThen(new WriteNotificationsStep()).andThen(new 
CommitPrimaryStep());
   
         firstStep.compose(cd).thenAccept(v -> cd.commitObserver.committed());
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services