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