apurtell commented on code in PR #5081:
URL: https://github.com/apache/hbase/pull/5081#discussion_r1126782232
##########
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();
+ takeUserRegionLock(tries);
Review Comment:
Do we need this as a parameter? Can we use an instance variable instead?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java:
##########
@@ -1123,12 +1127,26 @@ rpcControllerFactory, getMetaLookupPool(),
connectionConfig.getMetaReadRpcTimeou
}
}
- void takeUserRegionLock() throws IOException {
+ private long heldStartTime;
Review Comment:
Agree
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java:
##########
@@ -1123,12 +1127,26 @@ rpcControllerFactory, getMetaLookupPool(),
connectionConfig.getMetaReadRpcTimeou
}
}
- void takeUserRegionLock() throws IOException {
+ private long heldStartTime;
+
+ void takeUserRegionLock(int tries) throws IOException {
try {
long waitTime = connectionConfig.getMetaOperationTimeout();
+ long waitStartTime = 0;
+ 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) {
+ // successfully grabbed the lock, start timer of holding the lock
+ heldStartTime = EnvironmentEdgeManager.currentTime();
+ metrics.updateUserRegionLockWaiting(tries * waitTime + heldStartTime -
waitStartTime);
Review Comment:
Just directly measure and add up the time under lock.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -598,6 +611,24 @@ public void incrementServerOverloadedBackoffTime(long
time, TimeUnit timeUnit) {
overloadedBackoffTimer.update(time, timeUnit);
}
+ /** incr */
Review Comment:
+1, unit test required
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -443,6 +447,15 @@ protected Ratio getRatio() {
this.nsLookups = registry.counter(name(this.getClass(), NS_LOOKUPS,
scope));
this.nsLookupsFailed = registry.counter(name(this.getClass(),
NS_LOOKUPS_FAILED, scope));
+ this.userRegionLockTimeoutCount =
+ registry.counter(name(this.getClass(), "userRegionLockTimeoutCount",
scope));
+ this.userRegionLockWaitingTimer =
+ registry.timer(name(this.getClass(), "userRegionLockWaitingTimer",
scope));
+ this.userRegionLockHeldTimer =
+ registry.timer(name(this.getClass(), "userRegionLockHeldTimer", scope));
Review Comment:
We don't have a universal convention here but in my opinion the default time
unit is millisecond so we should not put the time unit into the metric name if
the metric's unit is milliseconds.
--
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]
