fluo-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] jkosh44 commented on issue #978: Use CompleteableFuture compose to centralize commit logic
Date Thu, 01 Jan 1970 00:00:00 GMT
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:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message