[GitHub] [hbase] joshelser commented on a change in pull request #2118: HBASE-24758 Avoid flooding replication source RSes logs when no sinks…

[GitBox <gi...@apache.org>]

joshelser commented on a change in pull request #2118:
URL: https://github.com/apache/hbase/pull/2118#discussion_r458916606



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/HBaseInterClusterReplicationEndpoint.java
##########
@@ -127,6 +127,7 @@
   private boolean dropOnDeletedTables;
   private boolean dropOnDeletedColumnFamilies;
   private boolean isSerial = false;
+  private long lastSinkFetchTime;

Review comment:
       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?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSinkManager.java
##########
@@ -150,9 +151,14 @@ public synchronized void reportSinkSuccess(SinkPeer sinkPeer) {
    */
   public synchronized void chooseSinks() {
     List<ServerName> slaveAddresses = endpoint.getRegionServers();
-    Collections.shuffle(slaveAddresses, ThreadLocalRandom.current());
-    int numSinks = (int) Math.ceil(slaveAddresses.size() * ratio);
-    sinks = slaveAddresses.subList(0, numSinks);
+    if(slaveAddresses==null){

Review comment:
       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.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/HBaseInterClusterReplicationEndpoint.java
##########
@@ -513,8 +514,14 @@ public boolean replicate(ReplicateContext replicateContext) {
 
     int numSinks = replicationSinkMgr.getNumSinks();
     if (numSinks == 0) {
-      LOG.warn("{} No replication sinks found, returning without replicating. "
-        + "The source should retry with the same set of edits.", logPeerId());
+      if((System.currentTimeMillis() - lastSinkFetchTime) >= (maxRetriesMultiplier*1000)) {
+        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.");

Review comment:
       nit, parameterized logging

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/HBaseInterClusterReplicationEndpoint.java
##########
@@ -513,8 +514,14 @@ public boolean replicate(ReplicateContext replicateContext) {
 
     int numSinks = replicationSinkMgr.getNumSinks();
     if (numSinks == 0) {
-      LOG.warn("{} No replication sinks found, returning without replicating. "
-        + "The source should retry with the same set of edits.", logPeerId());
+      if((System.currentTimeMillis() - lastSinkFetchTime) >= (maxRetriesMultiplier*1000)) {
+        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.");
+        lastSinkFetchTime = System.currentTimeMillis();
+      }
+      sleepForRetries("No sinks available at peer", sleepMultiplier);

Review comment:
       Might it be helpful to include which peer (in the case that we have multiple) here?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org