vli02 commented on code in PR #5081:
URL: https://github.com/apache/hbase/pull/5081#discussion_r1129907386
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java:
##########
@@ -1125,10 +1130,21 @@ rpcControllerFactory, getMetaLookupPool(),
connectionConfig.getMetaReadRpcTimeou
void takeUserRegionLock() throws IOException {
try {
+ long waitStartTime = 0;
Review Comment:
I think it's better to be initialized, I may get more comments like yours
from open source community if I don't initialize it, or java may complain it
during compliation.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java:
##########
@@ -1125,10 +1130,21 @@ rpcControllerFactory, getMetaLookupPool(),
connectionConfig.getMetaReadRpcTimeou
void takeUserRegionLock() throws IOException {
try {
+ long waitStartTime = 0;
long waitTime = connectionConfig.getMetaOperationTimeout();
+ if (metrics != null) {
+ metrics.updateUserRegionLockQueue(userRegionLock.getQueueLength());
+ waitStartTime = EnvironmentEdgeManager.currentTime();
+ }
if (!userRegionLock.tryLock(waitTime, TimeUnit.MILLISECONDS)) {
+ if (metrics != null) {
+ metrics.incrUserRegionLockTimeout();
+ }
throw new LockTimeoutException("Failed to get user region lock in" +
waitTime + " ms. "
+ " for accessing meta region server.");
+ } else if (metrics != null) {
Review Comment:
It's better to have in my opinion, it does not harm anything at least.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestMetaCache.java:
##########
@@ -459,6 +460,22 @@ 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();
+ assertEquals("Queue of userRegionLock should be updated twice.",
queueCount, 2);
+
+ long timeoutCount = metrics.getUserRegionLockTimeout().getCount();
+ assertEquals("Timeout of userRegionLock should happen once.",
timeoutCount, 1);
+
+ long waitingTimerCount =
metrics.getUserRegionLockWaitingTimer().getCount();
+ assertEquals("userRegionLock should be grabbed successfully once.",
waitingTimerCount, 1);
+
+ long heldTimerCount = metrics.getUserRegionLockHeldTimer().getCount();
+ assertEquals("userRegionLock should be held successfully once.",
heldTimerCount, 1);
+ double heldTime =
metrics.getUserRegionLockHeldTimer().getSnapshot().getMax();
+ assertTrue("Max held time should be greater than 2 seconds.", heldTime
>= 2E6);
Review Comment:
You are actually correct, it was always 2.046E9. I logged the time with
multiple rounds of testing in the very first time. Good catch. Thanks!
--
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]
