-
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-24758 Avoid flooding replication source RSes logs when no sinks… #2118
Conversation
… are available Change-Id: I642250ad3b9c958de93c7b34d5435c6523df4641
🎊 +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.
Looks good. A couple of comments.
LOG.warn( | ||
"No replication sinks found, returning without replicating. " | ||
+ "The source should retry with the same set of edits. Not logging this again for " | ||
+ "the next " + maxRetriesMultiplier + " seconds."); |
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.
nit, parameterized logging
@@ -127,6 +127,7 @@ | |||
private boolean dropOnDeletedTables; | |||
private boolean dropOnDeletedColumnFamilies; | |||
private boolean isSerial = false; | |||
private long lastSinkFetchTime; |
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.
JVM will initialize this to zero which is of consequence to the log message (will make sure that you get the LOG.warn
the first time).
Explicitly initialize it here so with a comment so we know that?
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.
+1
...a/org/apache/hadoop/hbase/replication/regionserver/HBaseInterClusterReplicationEndpoint.java
Show resolved
Hide resolved
Collections.shuffle(slaveAddresses, ThreadLocalRandom.current()); | ||
int numSinks = (int) Math.ceil(slaveAddresses.size() * ratio); | ||
sinks = slaveAddresses.subList(0, numSinks); | ||
if(slaveAddresses==null){ |
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.
Doesn't look like HBaseReplicationEndpoint ever returns null. Guarding against custom endpoint implementations?
We should expose getRegionServers
on a base-class or interface and explicitly say that we expect a non-null answer. Follow-on..
If easy, this would be good to write a quick unit test to cover this method.
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.
Doesn't look like HBaseReplicationEndpoint ever returns null. Guarding against custom endpoint implementations?
Yeah, that's indeed the case.
We should expose getRegionServers on a base-class or interface and explicitly say that we expect a non-null answer. Follow-on..
If easy, this would be good to write a quick unit test to cover this method.
So we can leave getRegionServers
at HBaseReplicationEndpoint, emphasize it should never return null. I'm not sure a UT for this would be effective, though. Custom implementations of HBaseReplicationEndpoint returning null would still blow it at runtime.
So maybe we can safely log warning for if(slaveAddresses.size()==0) ?
Yeah, can just leave that.
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.
One minor comment, looks good otherwise.
...r/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSinkManager.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Change-Id: I275a01a090472bf9dcf95f89db4407021f778151
🎊 +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.
Looks good. Up to you if you want to do a UT up front.
💔 -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.
+1
Some WAL related tests failed, but it's not clear if patch could be the reason. Let me retrigger the build. |
🎊 +1 overall
This message was automatically generated. |
Change-Id: I7e8b07e6215451bd288523b7c07e0104e90dcdd9
retest build |
🎊 +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. |
Latest UT failures seems unrelated, had the same passing locally. |
apache#2118) Signed-off-by: Josh Elser <elserj@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 8c0d7fa)
apache#2118) Signed-off-by: Josh Elser <elserj@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 8c0d7fa) Author: Wellington Chevreuil Reason: Supportability Ref: CDPD-15511 Change-Id: I7c3c0e421d4dc3d3a9e6255085eb6e796bc773f0
… are available