-
Notifications
You must be signed in to change notification settings - Fork 445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixes #558 use copy to avoid deadlock in tserver #559
Conversation
} | ||
|
||
Set<String> beginClearingUnusedLogs() { | ||
Set<String> doomed = new HashSet<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I raised this before, but hopefully I can make my objection more clear this time around:
"Doomed" is an extremely ambiguous concept (it conveys no information about the end state, only that it will be "unfortunate", whatever that means in any given context). Using it here, especially in already error prone WAL code, introduces unnecessary confusion to subsequent contributors/debuggers. It should either be clearly documented when it is declared, with a direct, complete, and unambiguous answer to the question "What does it mean to be 'doomed' here?" or avoided entirely. I prefer to avoid: If we mean "logs to clear" then let's just say what we mean and call the variable "logsToClear". If we mean something else, then let's use similarly simple language to convey what we mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted changes to this code since 1.9.1 in order to minimize what I had to consider. In this process I lost the doomed comment. I can make the variable name more clear.
It would be useful if someone else could double check my anaylsis of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I ran a ~1hr CI test w/ agitation against this branch and it went well. I can run longer test w/ and w/o agitation against RC2 starting Mon.
|
While looking into creating the release notes, I was getting confused trying to unravel and understand the #441 and #539 bugs fixes. Looking back, I have it all sorted out in my mind now. I am writing up a summary of my understanding in this comment. For each tablet there are three different sets of active WALogs. While a WALog exist in anyone of these sets it must not be garbage collected. The following are the three sets.
From Accumulo [1.8.0,1.9.0] only set 1 was consulted to determine what WALogs were active for a tablet. In #443 the code was changed to also consider set 2, however this may have introduced a possible deadlock in the code. So 1.9.1 consults sets 1 and 2, but may have a deadlock. This PR also considers set 3 and fixes the deadlock. So 1.9.2 consults sets 1,2,and 3 w/o having a deadlock. |
No description provided.