keith-turner commented on code in PR #4524:
URL: https://github.com/apache/accumulo/pull/4524#discussion_r1670934639
##########
core/src/main/java/org/apache/accumulo/core/fate/AbstractFateStore.java:
##########
@@ -298,7 +299,13 @@ private Optional<FateId> create(FateKey fateKey) {
@Override
public Optional<FateTxStore<T>> createAndReserve(FateKey fateKey) {
- FateId fateId = fateIdGenerator.fromTypeAndKey(getInstanceType(), fateKey);
+ // TODO 4131 not confident about this new implementation of
createAndReserve.
Review Comment:
The way this method previously worked and the way the code was structured it
was driven by the lock object being in AbstractFateStore. Now that there is no
longer a lock we want to implement the following algorithm for both stores
using conditional updates (@cshannon you added the fatekey feature, does the
following algorithm seem ok to you?).
1. var fateId = FateId fateId = fateIdGenerator.fromTypeAndKey(type(),
fateKey);
2. in the store conditionally create fateId setting the status,
reservation and fateKey atomically at creation time. The condition should check
that nothing currently exist for the fateId, its expected to be absent.
3. If the conditional update fails for some reason, read the fateId and see
if it already has the expected status, fateKey and reservation. If it does,
then probably running for a second time and already reserved. If it does not
then need to take the following actions.
1. If the fateId exists and has the same fateKey, but different status
(should have NEW status) or reservation than expected then return
Optional.empty(). This is the case where another thread has already created and
reserved using the fateKey so there is nothing to do.
2. If the fateId exists and has different fateKey or no fateKey then
this represents an unexpected collision ( this unexpected with the 128 bit
keys) so throw an exception in this case.
5. If the conditional update succeed , then we can return a non empty
Optional w/ a FateTxStore obj.
Now that the lock object is no longer needed in AbstractFateStore it may
make sense to completely push the full implementation of the
`createAndReserve` method into MetaFateStore and UserFateStore and have nothing
in AbstractFateStore for the method. The reason I am thinking this may be
better is because the algorithm above is based on conditional updates now and
how ZK and accumulo do conditional updates is very different. However there
may still be opportunities to share common code, so not sure of the best way to
structure the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
