-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-27466: Making metrics instance containing one or more connections. #4874
Conversation
metrics object of connections.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnection.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnection.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionFactory.java
Outdated
Show resolved
Hide resolved
Supplier<ThreadPoolExecutor> batchPool, Supplier<ThreadPoolExecutor> metaPool) { | ||
String scope = getScope(conn); | ||
MetricsConnection metrics; | ||
synchronized (METRICS_INSTANCES) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider a synchronized or concurrent map type instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if a synchronized or concurrent map can protect the entire block, I want this entire block to run in single thread mode. Especially in the deletion method below, the decrementing count, getting count, and remove it from the map have to be single threaded. @apurtell @d-c-manning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scope here should be unique per async connection object, is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the scope that you meant here is the scope of this code block, it is per async connection object, and the single metrics object which might be shared among multiple async connection objects. Please refer the comment in the deleteMetricsConnection() code block, too.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionFactory.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Let me retrigger the build. |
🎊 +1 overall
This message was automatically generated. |
Supplier<ThreadPoolExecutor> batchPool, Supplier<ThreadPoolExecutor> metaPool) { | ||
String scope = getScope(conn); | ||
MetricsConnection metrics; | ||
synchronized (METRICS_INSTANCES) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the scope that you meant here is the scope of this code block, it is per async connection object, and the single metrics object which might be shared among multiple async connection objects. Please refer the comment in the deleteMetricsConnection() code block, too.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java
Outdated
Show resolved
Hide resolved
This is worth giving a shot, this would avoid us having coarse-grained lock on the entire Map:
|
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Thanks for the sample code. I guess you missed the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left few nits, looks good overall.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
…ns. (#4874) (#4909) Signed-off-by: David Manning <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
…ns. (#4874) (#4909) Signed-off-by: David Manning <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
metrics object of connections.