d-c-manning commented on code in PR #5081:
URL: https://github.com/apache/hbase/pull/5081#discussion_r1128788688
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestMetaCache.java:
##########
@@ -459,6 +460,20 @@ public void testUserRegionLockThrowsException() throws
IOException, InterruptedE
assertTrue(client1.getException() instanceof LockTimeoutException
^ client2.getException() instanceof LockTimeoutException);
+
+ // obtain the client metrics
+ MetricsConnection metrics = conn.getConnectionMetrics();
+ long queueCount = metrics.getUserRegionLockQueue().getCount();
+ assertTrue(queueCount == 2);
Review Comment:
nit: use `assertEquals` for clearer error messages
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestMetaCache.java:
##########
@@ -459,6 +460,20 @@ public void testUserRegionLockThrowsException() throws
IOException, InterruptedE
assertTrue(client1.getException() instanceof LockTimeoutException
^ client2.getException() instanceof LockTimeoutException);
+
+ // obtain the client metrics
+ MetricsConnection metrics = conn.getConnectionMetrics();
+ long queueCount = metrics.getUserRegionLockQueue().getCount();
+ assertTrue(queueCount == 2);
+
+ long timeoutCount = metrics.getUserRegionLockTimeout().getCount();
+ assertTrue(timeoutCount == 1);
+
+ long waitingTimerCount =
metrics.getUserRegionLockWaitingTimer().getCount();
+ assertTrue(waitingTimerCount == 1);
+
+ long heldTimerCount = metrics.getUserRegionLockHeldTimer().getCount();
Review Comment:
we could assert something basic about the timer, since the comments here
suggest that we will hold the lock for 2 seconds. So we could assert that
`metrics.getUserRegionLockHeldTimer().getSnapshot().getMax() >= 2000`
milliseconds. (Probably better, just to avoid flaky tests, by subtracting some
epsilon in case of time travel or something weird..., i.e. >=1500ms?)
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java:
##########
@@ -1002,7 +1002,7 @@ private RegionLocations locateRegionInMeta(TableName
tableName, byte[] row, bool
}
// Query the meta region
long pauseBase = connectionConfig.getPauseMillis();
- takeUserRegionLock();
+ final long lockStartTime = takeUserRegionLock();
Review Comment:
I know I suggested this as a possible approach in the other comment, but it
also looks weird to me now 🙃
I'd be okay with it as-is, but it may also just make sense to leave the
return type as `void` and keep all the context within this method, as in:
```suggestion
takeUserRegionLock();
final long lockStartTime = EnvironmentEdgeManager.currentTime();
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
