Uh oh!
There was an error while loading. Please reload this page.
HBASE-22975 Add read and write QPS metrics at server level and table level - #615
Merged
Merged
Conversation
Apache-HBase
commented
Sep 12, 2019
💔 -1 overall
This message was automatically generated. |
ZhaoBQforce-pushed
the
HBASE-22975-master
branch
from
September 14, 2019 04:57
6356d5f to
f584db1CompareApache-HBase
commented
Sep 14, 2019
🎊 +1 overall
This message was automatically generated. |
| */ | ||
| @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.
| * 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
| 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?
| } | ||
| private static class TableMeters { | ||
| final Meter tableReadQueryPerSecondMeter; |
Contributor
There was a problem hiding this comment.
Naming, tableReadQueryMeter. There're few places in this class.
| 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.
| */ | ||
| @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?
| 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?
ContributorAuthor
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?
| this.writeRequestsCount.increment(); | ||
| if (rsServices != null && rsServices.getMetrics() != null) { | ||
| rsServices.getMetrics().updateServerWriteQueryPerSecond(this.htableDescriptor. |
| 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.
ZhaoBQforce-pushed
the
HBASE-22975-master
branch
from
September 17, 2019 16:11
f584db1 to
bf0d9beCompareApache-HBase
commented
Sep 17, 2019
💔 -1 overall
This message was automatically generated. |
| /** | ||
| * 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?
| public class RegionServerTableMetrics { | ||
| private final MetricsTableLatencies latencies; | ||
| private MetricsTableQueryMeter queryMeter; |
| } | ||
| 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.
| 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)?
| } | ||
| 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.
| 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
Reidddddd
left a comment
Contributor
There was a problem hiding this comment.
Good overall, but it needs some polish.
ZhaoBQforce-pushed
the
HBASE-22975-master
branch
from
September 19, 2019 11:26
bf0d9be to
ab8b784CompareApache-HBase
commented
Sep 19, 2019
💔 -1 overall
This message was automatically generated. |
| */ | ||
| @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
| boolean isSuccessful = false; | ||
| try { | ||
| this.writeRequestsCount.increment(); | ||
Contributor
There was a problem hiding this comment.
Please avoid unnecessary change.
| closeBulkRegionOperation(); | ||
| } | ||
| return isSuccessful ? storeFiles : null; |
Reidddddd
approved these changes
Sep 20, 2019
Apache-HBase
commented
Sep 20, 2019
💔 -1 overall
This message was automatically generated. |
ZhaoBQforce-pushed
the
HBASE-22975-master
branch
from
September 20, 2019 11:48
ab3c998 to
f26adfbCompareApache-HBase
commented
Sep 20, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 20, 2019
🎊 +1 overall
This message was automatically generated. |
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
Sign up for freeto 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