HBASE-22975 Add read and write QPS metrics at server level and table level #615
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
ZhaoBQ
force-pushed
the
HBASE-22975-master
branch
from
6356d5f to
f584db1
Compare
|
🎊 +1 overall
This message was automatically generated. |
Reidddddd
reviewed
Sep 16, 2019
| */ | ||
| @InterfaceAudience.Private | ||
| public interface MetricsTableLatencies { | ||
| public interface MetricsTableLatencies extends BaseSource { |
Contributor
There was a problem hiding this comment.
MetricsTableLatencies extends BaseSourceImpl which implements BaseSource.
Can't see the reason why MetricsTableLatencies extends this interface, again.
Reidddddd
reviewed
Sep 16, 2019
| * Query Per Second for a specific table in a RegionServer. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public interface MetricsTableQueryPerSecond { |
Contributor
There was a problem hiding this comment.
Naming, MetricsTableQueryMeter sounds more succinct
Reidddddd
reviewed
Sep 16, 2019
| import org.apache.yetus.audience.InterfaceAudience; | ||
|
|
||
| /** | ||
| * Query Per Second for a specific table in a RegionServer. |
Contributor
There was a problem hiding this comment.
For a specific table? which one? But it looks like target all tables in a RS?
Reidddddd
reviewed
Sep 16, 2019
| } | ||
|
|
||
| private static class TableMeters { | ||
| final Meter tableReadQueryPerSecondMeter; |
Contributor
There was a problem hiding this comment.
Naming, tableReadQueryMeter. There're few places in this class.
Reidddddd
reviewed
Sep 16, 2019
| public void updateTableReadQueryPerSecond(long count) { | ||
| tableReadQueryPerSecondMeter.mark(count); | ||
| } | ||
| public void updateTableReadQueryPerSecond() { |
Contributor
There was a problem hiding this comment.
Nit, a new empty line between methods.
Reidddddd
reviewed
Sep 16, 2019
| */ | ||
| @InterfaceAudience.Private | ||
| public class MetricsTableQueryPerSecondImpl implements MetricsTableQueryPerSecond { | ||
| private final HashMap<TableName,TableMeters> metersByTable = new HashMap<>(); |
Contributor
There was a problem hiding this comment.
Is it hashmap enough? Are there concurrent accesses?
Reidddddd
reviewed
Sep 16, 2019
| if (!initialized) { | ||
| this.writeRequestsCount.add(batchOp.size()); | ||
| if (rsServices != null && rsServices.getMetrics() != null) { | ||
| rsServices.getMetrics().updateServerWriteQueryPerSecond(this.htableDescriptor. |
Contributor
There was a problem hiding this comment.
What about increServerWriteQuery?
Contributor
Author
There was a problem hiding this comment.
Incre is usually used for simple numerical increments, but Meter is a complex data structure. What about updateWriteQueryMeter?
Reidddddd
reviewed
Sep 16, 2019
| this.writeRequestsCount.increment(); | ||
|
|
||
| if (rsServices != null && rsServices.getMetrics() != null) { | ||
| rsServices.getMetrics().updateServerWriteQueryPerSecond(this.htableDescriptor. |
Reidddddd
reviewed
Sep 16, 2019
| private MetricRegistry metricRegistry; | ||
| private Timer bulkLoadTimer; | ||
| private Meter serverReadQueryPerSecond; | ||
| private Meter serverWriteQueryPerSecond; |
Contributor
There was a problem hiding this comment.
What about serverReadQueryMeter and serverWriteQueryMeter.
ZhaoBQ
force-pushed
the
HBASE-22975-master
branch
from
f584db1 to
bf0d9be
Compare
|
💔 -1 overall
This message was automatically generated. |
Reidddddd
reviewed
Sep 19, 2019
|
|
||
| /** | ||
| * Implementation of {@link MetricsTableQueryMeter} to track query per second for one table in | ||
| * a RegionServer. |
Contributor
There was a problem hiding this comment.
Doc is wrong? Not for one table?
Reidddddd
reviewed
Sep 19, 2019
| public class RegionServerTableMetrics { | ||
|
|
||
| private final MetricsTableLatencies latencies; | ||
| private MetricsTableQueryMeter queryMeter; |
Reidddddd
reviewed
Sep 19, 2019
| } | ||
|
|
||
| private TableMeters getOrCreateTableMeter(String tableName) { | ||
| final TableName tn = TableName.valueOf(tableName); |
Contributor
There was a problem hiding this comment.
The upper layer passes down a TableName, but it is converted to String, here then it is converted back to TableName.
Reidddddd
reviewed
Sep 19, 2019
|
|
||
| private TableMeters getOrCreateTableMeter(String tableName) { | ||
| final TableName tn = TableName.valueOf(tableName); | ||
| return metersByTable.computeIfAbsent(tn, k -> new TableMeters(metricRegistry, tn)); |
Contributor
There was a problem hiding this comment.
The lambda is confusing. Parameter k is never used. It should be k -> new TableMeters(metricRegistry, k)?
Reidddddd
reviewed
Sep 19, 2019
| } | ||
| if (rsServices != null && rsServices.getMetrics() != null) { | ||
| rsServices.getMetrics().updateReadQueryMeter(getRegionInfo().getTable()); | ||
| } |
Contributor
There was a problem hiding this comment.
Is the place right? The readRequestsCount is after outResults fetched.
Reidddddd
reviewed
Sep 19, 2019
| if (rsServices != null && rsServices.getMetrics() != null) { | ||
| rsServices.getMetrics().updateWriteQueryMeter(this.htableDescriptor. | ||
| getTableName()); | ||
| } |
Contributor
There was a problem hiding this comment.
The write meter update should place after write execution, it is different from writeRequestsCount because execution time matters.
Reidddddd
requested changes
Sep 19, 2019
Contributor
Reidddddd
left a comment
Reidddddd
left a comment
There was a problem hiding this comment.
Good overall, but it needs some polish.
ZhaoBQ
force-pushed
the
HBASE-22975-master
branch
from
bf0d9be to
ab8b784
Compare
|
💔 -1 overall
This message was automatically generated. |
Reidddddd
reviewed
Sep 20, 2019
| */ | ||
| @InterfaceAudience.Private | ||
| public class MetricsTableQueryMeterImpl implements MetricsTableQueryMeter { | ||
| private final Map<TableName,TableMeters> metersByTable = new ConcurrentHashMap<>(); |
Contributor
There was a problem hiding this comment.
nit, space between TableName,TableMeters
Reidddddd
reviewed
Sep 20, 2019
| boolean isSuccessful = false; | ||
| try { | ||
| this.writeRequestsCount.increment(); | ||
|
|
Contributor
There was a problem hiding this comment.
Please avoid unnecessary change.
Reidddddd
reviewed
Sep 20, 2019
|
|
||
| closeBulkRegionOperation(); | ||
| } | ||
| return isSuccessful ? storeFiles : null; |
Reidddddd
approved these changes
Sep 20, 2019
|
💔 -1 overall
This message was automatically generated. |
ZhaoBQ
force-pushed
the
HBASE-22975-master
branch
from
ab3c998 to
f26adfb
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HBASE-22975