HBASE-23065 [hbtop] Top-N heavy hitter user and client drill downs #722

ankitsinghal · 2019-10-14T23:33:30Z

No description provided.

ankitsinghal · 2019-10-14T23:35:06Z

brfrn169

Left some comments for hbtop. Most of them are small nits. I have a question. It looks like this PR includes the changes of HBASE-15519, right? You are assuming that after committing the PR of HBASE-15519, we will commit this PR?

hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/mode/ClientModeStrategy.java

hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/mode/UserModeStrategy.java

hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/screen/top/TopScreenModel.java

hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/screen/top/TopScreenPresenter.java

hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/mode/ClientModeStrategy.java

hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/mode/ModeStrategy.java

hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/screen/top/TopScreenModel.java

brfrn169 · 2019-10-20T16:20:28Z

And don't we need ClientModeTest?

joshelser

Need to take this down locally and play with it too :)

Probably not a bad idea to also run this through a quick perf test to make sure no regressions come in (same way as the dependent user metrics change)

hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/mode/ClientModeStrategy.java

joshelser · 2019-10-21T14:45:37Z

hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/mode/ModeStrategyUtils.java

+   * @param countField Field which needs to be added with value 1 for each record
+   * @return records after selecting required fields and adding count field
+   */
+  public static List<Record> selectModeFieldsAndAddCountField(List<FieldInfo> fieldInfos,


Instead of doing this in one fatal swoop, could you split this method into two parts?

pseudo-code-y

.map(Record::select) .map(Record::addCountField)

I guess this would undo the optimization you're trying to make of copying Record objects?

Yeah, Let me see if I can make it more readable.

hbase-protocol-shaded/src/main/protobuf/ClusterStatus.proto

hbase-server/src/test/java/org/apache/hadoop/hbase/TestClientClusterMetrics.java

ankitsinghal · 2019-10-24T07:14:50Z

Thank you @brfrn169 and @joshelser for the review. Have pushed the commit addressing most of the review comments.

apurtell

One nit and a question. I defer comment on hbtop internal changes to @brfrn169

hbase-client/src/main/java/org/apache/hadoop/hbase/ServerMetricsBuilder.java

hbase-client/src/main/java/org/apache/hadoop/hbase/UserMetrics.java

ankitsinghal · 2019-10-30T02:53:31Z

Thanks a lot guys for the review @joshelser @brfrn169 @apurtell , have updated the pull request as per the review comments.

ankitsinghal · 2019-10-30T03:29:02Z

Need to take this down locally and play with it too :)

Probably not a bad idea to also run this through a quick perf test to make sure no regressions come in (same way as the dependent user metrics change)

./bin/ycsb run hbase20 -P workloads/workloada -s -threads 10 -p columnfamily=family

ycsb shows performance similar on single server cluster with and without patch.

without patch

[OVERALL], RunTime(ms), 52493
[OVERALL], Throughput(ops/sec), 19050.159068828223
[TOTAL_GCS_PS_Scavenge], Count, 73
[TOTAL_GC_TIME_PS_Scavenge], Time(ms), 123
[TOTAL_GC_TIME_%PS_Scavenge], Time(%), 0.23431695654658716
[TOTAL_GCS_PS_MarkSweep], Count, 1
[TOTAL_GC_TIME_PS_MarkSweep], Time(ms), 32
[TOTAL_GC_TIME%PS_MarkSweep], Time(%), 0.06096050902025032
[TOTAL_GCs], Count, 74
[TOTAL_GC_TIME], Time(ms), 155
[TOTAL_GC_TIME%], Time(%), 0.2952774655668375
[READ], Operations, 468618
[READ], AverageLatency(us), 518.8190018309156
[READ], MinLatency(us), 144
[READ], MaxLatency(us), 105983
[READ], 95thPercentileLatency(us), 815
[READ], 99thPercentileLatency(us), 1395
[READ], Return=OK, 468618
[READ], Return=NOT_FOUND, 31339
[CLEANUP], Operations, 20
[CLEANUP], AverageLatency(us), 122.7
[CLEANUP], MinLatency(us), 0
[CLEANUP], MaxLatency(us), 2081
[CLEANUP], 95thPercentileLatency(us), 263
[CLEANUP], 99thPercentileLatency(us), 2081
[UPDATE], Operations, 500043
[UPDATE], AverageLatency(us), 513.8964949014385
[UPDATE], MinLatency(us), 171
[UPDATE], MaxLatency(us), 118143
[UPDATE], 95thPercentileLatency(us), 780
[UPDATE], 99thPercentileLatency(us), 1448
[UPDATE], Return=OK, 500043
[READ-FAILED], Operations, 31339
[READ-FAILED], AverageLatency(us), 401.0351957624685
[READ-FAILED], MinLatency(us), 122
[READ-FAILED], MaxLatency(us), 22671
[READ-FAILED], 95thPercentileLatency(us), 633
[READ-FAILED], 99thPercentileLatency(us), 1195

With Patch

[OVERALL], RunTime(ms), 51625
[OVERALL], Throughput(ops/sec), 19370.46004842615
[TOTAL_GCS_PS_Scavenge], Count, 77
[TOTAL_GC_TIME_PS_Scavenge], Time(ms), 133
[TOTAL_GC_TIME_%PS_Scavenge], Time(%), 0.2576271186440678
[TOTAL_GCS_PS_MarkSweep], Count, 1
[TOTAL_GC_TIME_PS_MarkSweep], Time(ms), 25
[TOTAL_GC_TIME%PS_MarkSweep], Time(%), 0.048426150121065374
[TOTAL_GCs], Count, 78
[TOTAL_GC_TIME], Time(ms), 158
[TOTAL_GC_TIME%], Time(%), 0.30605326876513317
[READ], Operations, 383745
[READ], AverageLatency(us), 492.0543016847125
[READ], MinLatency(us), 119
[READ], MaxLatency(us), 124479
[READ], 95thPercentileLatency(us), 752
[READ], 99thPercentileLatency(us), 1380
[READ], Return=OK, 383745
[READ], Return=NOT_FOUND, 116557
[CLEANUP], Operations, 20
[CLEANUP], AverageLatency(us), 122.8
[CLEANUP], MinLatency(us), 0
[CLEANUP], MaxLatency(us), 2019
[CLEANUP], 95thPercentileLatency(us), 309
[CLEANUP], 99thPercentileLatency(us), 2019
[READ-FAILED], Operations, 116557
[READ-FAILED], AverageLatency(us), 420.47432586631436
[READ-FAILED], MinLatency(us), 122
[READ-FAILED], MaxLatency(us), 89279
[READ-FAILED], 95thPercentileLatency(us), 654
[READ-FAILED], 99thPercentileLatency(us), 1274
[UPDATE], Operations, 499698
[UPDATE], AverageLatency(us), 535.2963730092977
[UPDATE], MinLatency(us), 173
[UPDATE], MaxLatency(us), 135935
[UPDATE], 95thPercentileLatency(us), 801
[UPDATE], 99thPercentileLatency(us), 1561
[UPDATE], Return=OK, 499698

brfrn169

I tested this locally and found some bugs. Left some comments for that.

I think this feature should be great for HBase administrators. Thank you! @ankitsinghal

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java

hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/mode/ClientModeStrategy.java

hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/mode/UserModeStrategy.java

hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/screen/top/TopScreenModel.java

ankitsinghal · 2019-11-16T19:16:53Z

Thank you @brfrn169 for the review, Let me work on the review comments today.

ankitsinghal · 2019-11-17T23:08:48Z

@brfrn169 , have taken the review comments in the latest push. Let me know how it looks?

Apache-HBase · 2019-11-18T09:00:05Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 32s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+0 🆗	prototool	0m 1s	prototool was not available.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 7 new or modified test files.
		_ master Compile Tests _
+0 🆗	mvndep	0m 34s	Maven dependency ordering for branch
+1 💚	mvninstall	5m 19s	master passed
+1 💚	compile	3m 23s	master passed
+1 💚	checkstyle	3m 1s	master passed
+1 💚	shadedjars	4m 33s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	2m 18s	master passed
+0 🆗	spotbugs	4m 2s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	10m 25s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 15s	Maven dependency ordering for patch
+1 💚	mvninstall	4m 56s	the patch passed
+1 💚	compile	3m 20s	the patch passed
+1 💚	cc	3m 20s	the patch passed
+1 💚	javac	3m 20s	the patch passed
+1 💚	checkstyle	2m 59s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	shadedjars	4m 34s	patch has no errors when building our shaded downstream artifacts.
+1 💚	hadoopcheck	15m 39s	Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚	hbaseprotoc	3m 21s	the patch passed
+1 💚	javadoc	2m 17s	the patch passed
+1 💚	findbugs	11m 26s	the patch passed
		_ Other Tests _
+1 💚	unit	0m 42s	hbase-protocol-shaded in the patch passed.
+1 💚	unit	0m 32s	hbase-hadoop-compat in the patch passed.
+1 💚	unit	0m 41s	hbase-hadoop2-compat in the patch passed.
+1 💚	unit	0m 27s	hbase-protocol in the patch passed.
+1 💚	unit	1m 52s	hbase-client in the patch passed.
-1 ❌	unit	163m 15s	hbase-server in the patch failed.
+1 💚	unit	1m 7s	hbase-hbtop in the patch passed.
+1 💚	asflicense	3m 41s	The patch does not generate ASF License warnings.
		259m 15s

Subsystem	Report/Notes
Docker	Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-722/8/artifact/out/Dockerfile
GITHUB PR	#722
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile cc hbaseprotoc prototool
uname	Linux 65e4d40f91f6 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 GNU/Linux
Build tool	maven
Personality	/home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-722/out/precommit/personality/provided.sh
git revision	master / `a3efa59`
Default Java	1.8.0_181
unit	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-722/8/artifact/out/patch-unit-hbase-server.txt
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-722/8/testReport/
Max. process+thread count	4685 (vs. ulimit of 10000)
modules	C: hbase-protocol-shaded hbase-hadoop-compat hbase-hadoop2-compat hbase-protocol hbase-client hbase-server hbase-hbtop U: .
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-722/8/console
versions	git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

brfrn169

Left a comment that's a very small nit. It looks good to me 👍 Thanks! Ankit

brfrn169 · 2019-11-19T12:44:39Z

...btop/src/main/java/org/apache/hadoop/hbase/hbtop/screen/top/FilterDisplayModeScreenView.java

@@ -65,6 +65,7 @@ public void showFilters(List<RecordFilter> filters) {
    if (!filters.isEmpty()) {
      filtersString = String.join(" + ",
        filters.stream().map(f -> String.format("'%s'", f)).collect(Collectors.toList()));
+


We don't need this line :)

brfrn169 · 2019-12-08T14:07:54Z

Can I merge this? @ankitsinghal @joshelser @apurtell

joshelser · 2019-12-09T14:53:42Z

Can I merge this

OK by me. I know Ankit was traveling with intermittent access. I think he might be back soon, but I don't think you need to wait for him if you think this is good.

brfrn169 · 2019-12-16T15:41:43Z

Merged.

This comment has been minimized.

Sign in to view

brfrn169 requested changes Oct 20, 2019

View reviewed changes

joshelser reviewed Oct 21, 2019

View reviewed changes

ankitsinghal force-pushed the HBASE-23065 branch from 02de76b to 0918145 Compare October 24, 2019 06:23

This comment has been minimized.

Sign in to view

apurtell reviewed Oct 24, 2019

View reviewed changes

hbase-client/src/main/java/org/apache/hadoop/hbase/ServerMetricsBuilder.java Outdated Show resolved Hide resolved

hbase-client/src/main/java/org/apache/hadoop/hbase/UserMetrics.java Show resolved Hide resolved

This comment has been minimized.

Sign in to view

brfrn169 requested changes Nov 8, 2019

View reviewed changes

ankitsinghal force-pushed the HBASE-23065 branch from d8c56e0 to b5ff40a Compare November 17, 2019 22:51

HBASE-23065 [hbtop] Top-N heavy hitter user and client drill downs

8a720cd

ankitsinghal force-pushed the HBASE-23065 branch from b5ff40a to 8a720cd Compare November 17, 2019 23:08

This comment has been minimized.

Sign in to view

Fix checkstyle errors

a36cc92

brfrn169 approved these changes Nov 19, 2019

View reviewed changes

brfrn169 closed this Dec 16, 2019

HBASE-23065 [hbtop] Top-N heavy hitter user and client drill downs #722

HBASE-23065 [hbtop] Top-N heavy hitter user and client drill downs #722

Uh oh!

Conversation

ankitsinghal commented Oct 14, 2019

Uh oh!

ankitsinghal commented Oct 14, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

brfrn169 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brfrn169 commented Oct 20, 2019

Uh oh!

joshelser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joshelser Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

ankitsinghal Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ankitsinghal commented Oct 24, 2019

Uh oh!

This comment has been minimized.

apurtell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ankitsinghal commented Oct 30, 2019

Uh oh!

ankitsinghal commented Oct 30, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

brfrn169 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ankitsinghal commented Nov 16, 2019

Uh oh!

ankitsinghal commented Nov 17, 2019

Uh oh!

This comment has been minimized.

Apache-HBase commented Nov 18, 2019

Uh oh!

brfrn169 left a comment

Choose a reason for hiding this comment

Uh oh!

brfrn169 Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

brfrn169 commented Dec 8, 2019

Uh oh!

joshelser commented Dec 9, 2019

Uh oh!

brfrn169 commented Dec 16, 2019

Uh oh!

Uh oh!