-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-26768 Avoid unnecessary replication suspending in RegionReplica… #4127
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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 think we could just move the checks in onComplete method into the synchronized block?
And do we really need to expose so many internal things just for writing a UT? Just expose the entries out, synchronized all the time to block the onComplete method, and then trigger a flush? If synchronized is not easy to check whether there are waiters, then you could change to use a Lock.
@RestrictedApi(explanation = "Should only be called in tests", link = "", | ||
allowedOnPath = ".*/src/test/.*") | ||
Queue<SinkEntry> getEntries() { | ||
synchronized (entries) { |
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.
We just return the entries out so I do not get the point on 'synchronized (entries)' here...
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.
We just return the entries out so I do not get the point on 'synchronized (entries)' here...
That method is only for test, because entries
is mutated under synchronized, so here I get entries
to check it size also synchronized, but remove the synchronized is also ok, because when I check entries
in the UTs, there is no operations, use synchronized here just for absolute safety, I could remove it.
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@Apache9 , thank you very much for suggestions.
What do you mean? could you please explain more? you mean put the for (Map.Entry<Integer, MutableObject> entry : replica2Error.entrySet()) {
I expose these four internal things just for check and assert more internal states are as expected in the UT, in fact some checks is not really necessary for this UT, just check for failedReplicas is enough, I could remove unnecessary check and corresponding get internal state if you think exposing these internal state is improper. |
Yes, I mean put the code where we check for sequence ids into the synchronized block too, they are all in memory operations, should not effect performance too much. And if a UT is really complicated, I suggest that we just add more comments to explain why we need to synchronized all the checks on sequence ids, instead of exposing so many internal things... Thanks. |
🎊 +1 overall
This message was automatically generated. |
@Apache9 , I have modified the code. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
…tionSink