[GitHub] [hbase] bharathv commented on a change in pull request #3018: HBASE-25627: HBase replication should have a metric to represent if the source is stuck getting initialized

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

bharathv commented on a change in pull request #3018:
URL: https://github.com/apache/hbase/pull/3018#discussion_r589578343



##########
File path: hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/replication/regionserver/MetricsReplicationSourceSource.java
##########
@@ -80,4 +79,7 @@
   long getEditsFiltered();
   void setOldestWalAge(long age);
   long getOldestWalAge();
+  void incrSourceInitializing();
+  void decrSourceInitializing();
+  int getSourceInitializing();

Review comment:
       just make it long, why down cast?

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSource.java
##########
@@ -465,6 +465,22 @@ public synchronized UUID getPeerUUID() {
 
   }
 
+  /**
+   * Bad Endpoint with failing connection to peer on demand.
+   */
+  public static class BadReplicationEndpoint extends DoNothingReplicationEndpoint {
+    static boolean failing = true;
+
+    @Override
+    public synchronized UUID getPeerUUID() {
+      if(failing) {

Review comment:
       nit: if ( ... space missing.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
##########
@@ -578,7 +578,17 @@ private void initialize() {
     for (String walGroupId: logQueue.getQueues().keySet()) {
       tryStartNewShipper(walGroupId);
     }
-    this.startupOngoing.set(false);
+    setSourceStartupStatus(false);
+  }
+
+  private synchronized void setSourceStartupStatus(boolean initializing) {
+    if (initializing) {

Review comment:
       nit: to condense to fewer lines.
   
   startupOnging.set(initializing);
   initializing ? metrics.incrementSourceInitializing() : metrics.decr...

##########
File path: hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/replication/regionserver/MetricsReplicationSourceSource.java
##########
@@ -49,8 +49,7 @@
   public static final String SOURCE_COMPLETED_LOGS = "source.completedLogs";
   public static final String SOURCE_COMPLETED_RECOVERY_QUEUES = "source.completedRecoverQueues";
   public static final String SOURCE_FAILED_RECOVERY_QUEUES = "source.failedRecoverQueues";
-  /* Used to track the age of oldest wal in ms since its creation time */
-  String OLDEST_WAL_AGE = "source.oldestWalAge";
+  public static final String SOURCE_INITIALIZING = "source.initializing";

Review comment:
       s/initalizing/numInitializing ? (Otherwise hard to interpret what it means without looking at the code)
   
   Also mind adding a quick comment? (that it is useful for tracking sources that are stuck during initialization, especially cases like peer connection issues etc)




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