[GitHub] [hbase] bharathv commented on a change in pull request #2585: HBASE-25217 [Metrics] Add metrics for Call in IPC response queue

[GitBox]

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



##
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
##
@@ -1254,8 +1255,10 @@ private boolean processAllResponses(final Connection 
connection) throws IOExcept
   if (call == null) {
 return true;
   }
+  metrics.removeCallFromResponseQueue(call.response.getRemaining());

Review comment:
   Oh I see, you only want to account for the ones that are actually 
queued. Ok.

##
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
##
@@ -1254,8 +1255,10 @@ private boolean processAllResponses(final Connection 
connection) throws IOExcept
   if (call == null) {
 return true;
   }
+  metrics.removeCallFromResponseQueue(call.response.getRemaining());
   if (!processResponse(call)) {
 connection.responseQueue.addFirst(call);
+metrics.addCallToResponseQueue(call.response.getRemaining());

Review comment:
   Isn't the right way to do this something like..
   
   ```
before = call.response.getRemaining();  <=== 1
 if (!processResponse(call)) {
  after = call.response.getRemaining();
  metrics.decr(after-before);  <=== 2
   } else {
  metrics.decr(before);
   }
   
   ```
   The difference is you are not decrementing until the bytes are actually 
flushed, otherwise if a metrics poller polls between (1) and (2), there is some 
weird state in between, perhaps ^^ is more desirable for you.





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:
us...@infra.apache.org

[GitHub] [hbase] bharathv commented on a change in pull request #2585: HBASE-25217 [Metrics] Add metrics for Call in IPC response queue

[GitBox]

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



##
File path: 
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSource.java
##
@@ -91,6 +91,10 @@
   String NUM_LIFO_MODE_SWITCHES_NAME = "numLifoModeSwitches";
   String NUM_LIFO_MODE_SWITCHES_DESC = "Total number of calls in general queue 
which " +
 "were served from the tail of the queue";
+  String NUM_CALL_RESPONSE_QUEUE_NAME = "numCallsInResponseQueue";
+  String NUM_CALL_RESPONSE_QUEUE_DESC = "Number of calls in response queue.";
+  String NUM_SIZE_RESPONSE_QUEUE_NAME = "numSizeInResponseQueue";
+  String NUM_SIZE_RESPONSE_QUEUE_DESC = "Size in response queue.";

Review comment:
   nit: .. of...

##
File path: 
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSource.java
##
@@ -91,6 +91,10 @@
   String NUM_LIFO_MODE_SWITCHES_NAME = "numLifoModeSwitches";
   String NUM_LIFO_MODE_SWITCHES_DESC = "Total number of calls in general queue 
which " +
 "were served from the tail of the queue";
+  String NUM_CALL_RESPONSE_QUEUE_NAME = "numCallsInResponseQueue";
+  String NUM_CALL_RESPONSE_QUEUE_DESC = "Number of calls in response queue.";
+  String NUM_SIZE_RESPONSE_QUEUE_NAME = "numSizeInResponseQueue";

Review comment:
   nit: SIZE_OF_RESPONSE_QUEUE and sizeOfResponseQueue ? num + size sounds 
confusing..

##
File path: 
hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java
##
@@ -41,7 +40,8 @@
   private final MutableFastCounter authenticationFallbacks;
   private final MutableFastCounter sentBytes;
   private final MutableFastCounter receivedBytes;
-
+  private final MutableFastCounter numCallInResponseQueue;

Review comment:
   I think MutableGaugeLong is the right type for this.. Counters are meant 
to be always increasing where as a guage can go up and down...

##
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
##
@@ -1254,8 +1255,10 @@ private boolean processAllResponses(final Connection 
connection) throws IOExcept
   if (call == null) {
 return true;
   }
+  metrics.removeCallFromResponseQueue(call.response.getRemaining());

Review comment:
   Why add in multiple places, looks like processResponse(call) is the 
place where the actual change is happening, so move most updates to that method?
   
 if (!call.response.hasRemaining()) {
   call.done();
  // Remove here <
   return true;
 } else {
  // update here <=
   return false; // Socket can't take more, we will have to come back.
 }





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:
us...@infra.apache.org