Skip to content

HBASE-11062 hbtop - #476

Merged
brfrn169 merged 1 commit into
apache:masterfrom
brfrn169:HBASE-11062
Sep 7, 2019
Merged

HBASE-11062 hbtop#476
brfrn169 merged 1 commit into
apache:masterfrom
brfrn169:HBASE-11062

Conversation

@brfrn169

Copy link
Copy Markdown
Member

I removed the dependency for Lanterna to remove the license problem.

We can run hbtop by running hbase top command, and press h key in the top screen for the help screen.

For the details of hbtop, this is the presentation in NoSQL day 2019 (the name is changed from htop to hbtop):
https://dataworkssummit.com/nosql-day-2019/session/supporting-apache-hbase-troubleshooting-and-supportability-improvements/

@saintstacksaintstack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Put some images up in the JIRA so we can see how it looks. How did you come up with this body of work. Have you done terminal UI in the past? You did a mountain of work. Good on you.

Comment threadhbase-assembly/pom.xml
Comment threadhbase-hbtop/pom.xml Outdated
@brfrn169

Copy link
Copy Markdown
MemberAuthor

Thank you for reviewing! @saintstack

Put some images up in the JIRA so we can see how it looks.

Sure, will update the Jira.

How did you come up with this body of work. Have you done terminal UI in the past?

I have never done terminal UI in the past actually. And I thought about using Lanterna (https://github.com/mabe02/lanterna) to build the terminal UI, but I faced the license problem (Lanterna is LGPL License that's incompatible with Apache License. See mabe02/lanterna#415 for the details). So I decided not to use Lanterna and to make the terminal UI by myself.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@brfrn169

Copy link
Copy Markdown
MemberAuthor

I added README (https://github.com/brfrn169/hbase/blob/HBASE-11062/hbase-hbtop/README.md) in the hbtop module so that reviewers can see the details of hbtop.

Can you please review this PR when you get a chance? @saintstack@Apache9@apurtell@joshelser@busbey

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

Comment threadbin/hbase
Comment threadhbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/Record.java Outdated
@joshelser

Copy link
Copy Markdown
Member

Regarding the issue where the byte-sizes don't get rolled up to the next unit, I added a logic to do it in FieldValue

Looks good!

I made Record class immutable in the latest commit.

Left a new comment (throw UnsupportedOperationException instead of letting it throw an exception via the ImmutableMap), but that doesn't need to block getting this in.

If we think this is ready to go, I'll test it locally again.

@brfrn169

Copy link
Copy Markdown
MemberAuthor

the easiest way is to squash your commits and force push to your feature branch.

I did do this.

Left a new comment (throw UnsupportedOperationException instead of letting it throw an exception via the ImmutableMap)

And, I changed the patch for this Josh's comment.

Could you please let me know if there's something I need to do?

@joshelserjoshelser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will re-test it today, but LGTM

@Apache-HBase

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
0reexec49Docker mode activated.
_ Prechecks _
+1dupname2No case conflicting files found.
0shelldocs2Shelldocs was not available.
+1hbaseanti0Patch does not have any anti-patterns.
+1@author1The patch does not contain any @author tags.
+1test4tests0The patch appears to include 22 new or modified test files.
_ master Compile Tests _
0mvndep36Maven dependency ordering for branch
+1mvninstall314master passed
+1compile186master passed
+1checkstyle157master passed
+1shadedjars279branch has no errors when building our shaded downstream artifacts.
+1javadoc194master passed
0spotbugs33Used deprecated FindBugs config; considering switching to SpotBugs.
0findbugs33branch/hbase-assembly no findbugs output file (findbugsXml.xml)
_ Patch Compile Tests _
0mvndep16Maven dependency ordering for patch
+1mvninstall313the patch passed
+1compile186the patch passed
+1javac186the patch passed
+1checkstyle142the patch passed
+1shellcheck3There were no new shellcheck issues.
+1whitespace0The patch has no whitespace issues.
+1xml5The patch has no ill-formed XML file.
+1shadedjars295patch has no errors when building our shaded downstream artifacts.
+1hadoopcheck992Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1javadoc202the patch passed
0findbugs33hbase-assembly has no data from findbugs
_ Other Tests _
+1unit14862root in the patch passed.
+1asflicense112The patch does not generate ASF License warnings.
20620
SubsystemReport/Notes
DockerClient=19.03.1 Server=19.03.1 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-476/71/artifact/out/Dockerfile
GITHUB PR#476
Optional Testsdupname asflicense shellcheck shelldocs javac javadoc unit shadedjars hadoopcheck xml compile spotbugs findbugs hbaseanti checkstyle
unameLinux 5285cdcfbc6f 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 GNU/Linux
Build toolmaven
Personality/home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-476/out/precommit/personality/provided.sh
git revisionmaster / dfe5280
Default Java1.8.0_181
Test Resultshttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-476/71/testReport/
Max. process+thread count5497 (vs. ulimit of 10000)
modulesC: hbase-hbtop . hbase-assembly U: .
Console outputhttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-476/71/console
versionsgit=2.11.0 maven=2018-06-17T18:33:14Z) shellcheck=0.4.4 findbugs=3.1.11
Powered byApache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@brfrn169

Copy link
Copy Markdown
MemberAuthor

It looks like the QA is okay.

@saintstack Could you please take a look at this? As Josh, Sean and Andrew have approved it, If you are okay with it, I will commit it. Thanks.

@saintstacksaintstack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skimmed. Approved. My two concerns -- that we rely on a lib for terminal and console display and that this better sits in hbase-operator-tools -- saw good discussion w/ resolution on the first being that we'll pull in libs as/if we trip over them and on the latter, while a few of us might think it sits better in hbase-operator-tools, the maintainer would prefer core. Suggest you cast the README as a blog post for hbase.apache.org. The README is complete and this is a nice feature that folks will be excited about.

Comment threadhbase-hbtop/README.md
Comment threadhbase-hbtop/pom.xml Outdated
Comment threadhbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/HBTop.java Outdated
@brfrn169

Copy link
Copy Markdown
MemberAuthor

Thank you for approving this PR, guys. I will commit it. Thanks!

Suggest you cast the README as a blog post for hbase.apache.org. The README is complete and this is a nice feature that folks will be excited about.

@saintstack Thank you for suggesting this. I'd love to do it. But, I don't know how to post an article on hbase.apache.org. Could you please instruct me or give me documentation for it?

@saintstack

Copy link
Copy Markdown
Contributor

@brfrn169https://blogs.apache.org/roller-ui/menu.rol is how you log on to the blog. I think you have to be added. It might be a PMC chair thing... checking. Otherwise, open issue, put text up in it and we can go back and forth on it and I can post if we don't get you your own account. Doesn't have to be much. Just your README cast as a blog post. Your README has nice stuff in it.

Signed-off-by: Stack <stack@apache.org>
Signed-off-by: Sean Busbey <busbey@apache.org>
Signed-off-by: Josh Elser <elserj@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
@brfrn169
brfrn169 merged commit 78bae9e into apache:masterSep 7, 2019
@brfrn169

brfrn169 commented Sep 7, 2019

Copy link
Copy Markdown
MemberAuthor

https://blogs.apache.org/roller-ui/menu.rol is how you log on to the blog. I think you have to be added. It might be a PMC chair thing... checking. Otherwise, open issue, put text up in it and we can go back and forth on it and I can post if we don't get you your own account. Doesn't have to be much. Just your README cast as a blog post. Your README has nice stuff in it.

Sure, thanks. @saintstack
Please let me know if I need to open a new issue for it.

@Apache-HBase

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
💙reexec0m 39sDocker mode activated.
_ Prechecks _
💚dupname0m 1sNo case conflicting files found.
💙shelldocs0m 1sShelldocs was not available.
💙markdownlint0m 0smarkdownlint was not available.
💚hbaseanti0m 0sPatch does not have any anti-patterns.
💚@author0m 1sThe patch does not contain any @author tags.
💚test4tests0m 0sThe patch appears to include 22 new or modified test files.
_ master Compile Tests _
💙mvndep0m 47sMaven dependency ordering for branch
💚mvninstall5m 13smaster passed
💚compile3m 9smaster passed
💚checkstyle2m 30smaster passed
💚shadedjars4m 41sbranch has no errors when building our shaded downstream artifacts.
💚javadoc3m 12smaster passed
💙spotbugs0m 32sUsed deprecated FindBugs config; considering switching to SpotBugs.
💙findbugs0m 32sbranch/hbase-assembly no findbugs output file (findbugsXml.xml)
_ Patch Compile Tests _
💙mvndep0m 22sMaven dependency ordering for patch
💚mvninstall5m 9sthe patch passed
💚compile3m 3sthe patch passed
💚javac3m 3sthe patch passed
💚checkstyle2m 49sthe patch passed
💚shellcheck0m 2sThere were no new shellcheck issues.
💚whitespace0m 0sThe patch has no whitespace issues.
💚xml0m 4sThe patch has no ill-formed XML file.
💚shadedjars4m 44spatch has no errors when building our shaded downstream artifacts.
💚hadoopcheck16m 37sPatch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
💚javadoc3m 49sthe patch passed
💙findbugs0m 38shbase-assembly has no data from findbugs
_ Other Tests _
💚unit260m 10sroot in the patch passed.
💚asflicense2m 8sThe patch does not generate ASF License warnings.
360m 50s
SubsystemReport/Notes
DockerClient=19.03.1 Server=19.03.1 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-476/72/artifact/out/Dockerfile
GITHUB PR#476
Optional Testsdupname asflicense shellcheck shelldocs javac javadoc unit shadedjars hadoopcheck xml compile markdownlint spotbugs findbugs hbaseanti checkstyle
unameLinux 179af4f63f9e 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 GNU/Linux
Build toolmaven
Personality/home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-476/out/precommit/personality/provided.sh
git revisionmaster / 51b6ce3
Default Java1.8.0_181
Test Resultshttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-476/72/testReport/
Max. process+thread count5537 (vs. ulimit of 10000)
modulesC: hbase-hbtop . hbase-assembly U: .
Console outputhttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-476/72/console
versionsgit=2.11.0 maven=2018-06-17T18:33:14Z) shellcheck=0.4.4 findbugs=3.1.11
Powered byApache Yetus 0.11.0 https://yetus.apache.org

This message was automatically generated.

asfgit pushed a commit that referenced this pull request Sep 7, 2019
Signed-off-by: Stack <stack@apache.org>
Signed-off-by: Sean Busbey <busbey@apache.org>
Signed-off-by: Josh Elser <elserj@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit that referenced this pull request Sep 7, 2019
Signed-off-by: Stack <stack@apache.org>
Signed-off-by: Sean Busbey <busbey@apache.org>
Signed-off-by: Josh Elser <elserj@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit that referenced this pull request Sep 7, 2019
Signed-off-by: Stack <stack@apache.org>
Signed-off-by: Sean Busbey <busbey@apache.org>
Signed-off-by: Josh Elser <elserj@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
Comment threadbin/hbase
elif [ "$COMMAND" = "completebulkload" ] ; then
CLASS='org.apache.hadoop.hbase.tool.BulkLoadHFilesTool'
elif [ "$COMMAND" = "hbtop" ] ; then
CLASS='org.apache.hadoop.hbase.hbtop.HBTop'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's with all this gymnastics building a custom class path? Looks like a land mine waiting to trip up someone who wants to upgrade our dependencies.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any command that needs something beyond our basics (either client or normal service process) has to whitelist the extras it needs.

hbtop largely just needs the shaded client jar, but it has some third-party stuff it uses directly, hence the extra add on.

I think this is the best of not great options given how we currently do packaging.

If you want to discuss improvements to how we do command packaging it's something I'm interested in. A DISCUSS thread or an umbrella jira would probably be a better venue for that

infraio pushed a commit to infraio/hbase that referenced this pull request Aug 17, 2020
Signed-off-by: Stack <stack@apache.org>
Signed-off-by: Sean Busbey <busbey@apache.org>
Signed-off-by: Josh Elser <elserj@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
infraio pushed a commit to infraio/hbase that referenced this pull request Aug 17, 2020
symat pushed a commit to symat/hbase that referenced this pull request Feb 17, 2021
Signed-off-by: Stack <stack@apache.org>
Signed-off-by: Sean Busbey <busbey@apache.org>
Signed-off-by: Josh Elser <elserj@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
(cherry picked from commit 44dea25)
Change-Id: I0418ecf8422fc04d2a3e693b359416372fc0f6d6
symat pushed a commit to symat/hbase that referenced this pull request Feb 17, 2021
(cherry picked from commit caef72c)
Change-Id: I1cad7c43aaa0345d5fa357f7cd273aa260462773
@zhugelianglongming

Copy link
Copy Markdown

I removed the dependency for Lanterna to remove the license problem.

Hi, I'm curious about the specific license problem, can you tell me more about "Apache depends on LGPL"?

@brfrn169

Copy link
Copy Markdown
MemberAuthor

@zhugelianglongming We should not include the LGPL license within Apache products as mentioned in the following page:
http://www.apache.org/legal/resolved.html#category-x

You can also follow the following issue for the details:
mabe02/lanterna#415

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@brfrn169@Apache-HBase@saintstack@joshelser@busbey@apurtell@zhugelianglongming@ndimiduk