Improvements to reservation column in FATE table and bug fix #4992
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Makes it so the reservation column is created on reservation and deleted on unreservation (no longer store an unreserved value in the column) * Addresses a bug with MultipleStoresIT.testDeadReservationsCleanup() (ZooUtil.LockID was missing equals() and hashCode()) closes apache#4907
keith-turner
approved these changes
Oct 18, 2024
Contributor
keith-turner
left a comment
keith-turner
left a comment
There was a problem hiding this comment.
Nice find with the LockId problem.
|
|
||
| FateMutator.Status status = newMutator(fateId).putReservedTx(reservation).tryMutate(); | ||
| // requiring any status prevents creating an entry if the fate id doesn't exist | ||
| FateMutator.Status status = |
Contributor
There was a problem hiding this comment.
Nice comment here. Interesting, I suppose the status check was not needed previously because it was assumed the reservation column always existed if the fate operation existed.
Member
Author
There was a problem hiding this comment.
Yeah, before this, putReservedTx only worked if the reservation column had a NOT_RESERVED value, which was only set on creation and unreservation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR
MultipleStoresIT.testDeadReservationsCleanup()(ZooUtil.LockIDwas missingequals()andhashCode()) (see NOTE)NOTE about original bug when I first attempted these changes:
After making the reservation column changes, I tested
testDeadReservationsCleanup()to see if I could recreate the same bug noted in this thread #4524 (comment) (TLDR - <4 transactions would show as being reserved, even though 4 threads were working on 4 transactions). I was able to recreate the bug (extremely rarely until I messed with theDeadReservationCleanertimings), but found that it is not related to the reservation column changes and is instead just a bug with the test. I'm not sure why I only saw this bug with the column changes the first go-around, but regardless it's fixed now. I was concerned if this test was identifying a bug with the code, but turns out it was just a problem with the test. The issue was the dead reservation cleaner was removing transactions that weren't dead because theisLockHeldpredicate being passed was:final Predicate<ZooUtil.LockID> isLockHeld = liveLocks::contains;where
final Set<ZooUtil.LockID> liveLocks = new HashSet<>();ZooUtil.LockIDdid not haveequals()orhashCode()methods resulting in transactions being unexpectedly unreserved by the dead reservation cleaner.The real code (the Manager code) never calls equals on the LockID object, so this was just a bug with the test.
closes #4907