kevinrr888 commented on PR #4524: URL: https://github.com/apache/accumulo/pull/4524#issuecomment-2246036407
@keith-turner, @DomGarguilo I have addressed all of the review changes, and this PR is ready for re-review. However, some tests are no longer passing (e.g., ComprehensiveIT). I believe this is an issue with my new implementation of createAndReserve. I believe the issue is with the UserFateStore implementation, but could be both. Posting this new commit now to get the other changes out, and to potentially receive feedback on what might be wrong with createAndReserve(). I will continue to look into what the issue might be in the meantime. Please take a look at my new review comments and my new commit message: - Stricter check of the column value for fate reservations. If anything unexpected is seen, an error is now thrown. - Simplified MetaFateStore.getActiveReservations() to only read from ZooKeeper once. - Combined the two scans that were done in AbstractFateStore.runnable() into one. This meant adding FateReservation to FateIdStatus and refactoring Meta/UserFateStore.getTransactions(). - No longer use/store a string representation of the FateReservation (was used in UserFateStore). Now, only the serialized value is used. This keeps the usage of FateReservation consistent across Meta and UserFateStore. It was also unneccessary, so simplifies code. - Moved AbstractFateStore.createAndReserve() implementation into Meta and UserFateStore, and rewrote the impl for each to work with the new way reservations are stored. This also allowed me to delete methods that were only used by AFS.createAndReserve(): create(FateId, FateKey), getStatusAndKey(FateId), create(FateKey). - Fixed how concurrentStatusChangeCallers was decremented in AbstractFateStore.waitForStatusChange() - Small change to MetaFateStore.deleteDeadReservations() to avoid reading from ZK unnecessarily - Added isReservedBy() method to MetaFateStore.NodeValue to avoid code duplication and make the code more clear. - Since the FateIdStatus now has the FateReservation, realized Meta and UserFateStore.getActiveReservations() could now be simplified to just call list(). This also made the impls the same, so moved to AbstractFateStore. This also made me realize that I had put getActiveReservations() method signature in FateStore, but would be better suited for ReadOnlyFateStore, so moved it there. - Deleted FateStore.isReserved(FateId)... No longer needed/used - Moved UNKNOWN status check in AbstractFateStore.reserve() into waiting loop - Now log when a dead reservation is detected and deleted - Minor change to Fate: no longer create the executor for the dead reservation cleaner if it's not going to be used -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
