Skip to content

Setup testcontainers for hbase [tp-tests] #2576

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.

Sign up for GitHub

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

Jump to bottom
Merged
farodin91 merged 1 commit into from
Apr 24, 2021
Merged

Setup testcontainers for hbase [tp-tests] #2576

farodin91 merged 1 commit into from
Apr 24, 2021

Conversation

@farodin91
Copy link
Contributor

@farodin91 farodin91 commented Apr 17, 2021 •
edited
Loading

Fixes #1474

  • Add docker container for hbase tests
  • Merge hbase maven projects into one
  • Update hbase from 2.1.5 to 2.2.7
  • Update hadoop from 2.7.7 to 2.8.5

Follow up:

  • Cleanup hbase code base more
  • activate release tests for hbase

Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there an issue associated with this PR? Is it referenced in the commit message?
  • Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you written and/or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
  • If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

@farodin91 farodin91 requested review from li-boxuan and porunov April 17, 2021 21:50
@farodin91
Copy link
Contributor Author

farodin91 commented Apr 17, 2021

@li-boxuan @porunov I would like to get some feedback already.

@farodin91 farodin91 force-pushed the testcontainers-hbase branch 2 times, most recently from 65cb917 to 7d2d426 Compare April 17, 2021 21:54
@janusgraph-bot janusgraph-bot added the cla: external Externally-managed CLA label Apr 17, 2021
@farodin91 farodin91 force-pushed the testcontainers-hbase branch from 7d2d426 to 273db27 Compare April 18, 2021 06:53
@li-boxuan li-boxuan requested review from jerryjch and mbrukman April 18, 2021 13:13
@li-boxuan
Copy link
Member

li-boxuan commented Apr 18, 2021

Unfortunately, I know very little about HBase and I know nothing about how JanusGraph integrates with HBase. Just requested two more reviewers and hopefully, someone capable could review.

farodin91
farodin91 commented Apr 18, 2021
View reviewed changes
li-boxuan
li-boxuan reviewed Apr 20, 2021
View reviewed changes
Copy link
Member

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

As I said, I know very little about HBase, so my reviews are mostly nitpicks about code smell and typos.

One question: why did we need hbase-parent module as a shim for hbase 1 and 2, and why do we not need them now?

@farodin91
Copy link
Contributor Author

farodin91 commented Apr 20, 2021

Shim was removed during the removal of the support of hbase 0. combat class should be removed in a follow up pr.

I merged in this PR because it was much easier for me to identify what is needed what not.

@farodin91 farodin91 force-pushed the testcontainers-hbase branch 2 times, most recently from 05f500f to 5eded53 Compare April 20, 2021 21:33
@farodin91 farodin91 marked this pull request as ready for review April 20, 2021 21:33
@farodin91
Copy link
Contributor Author

farodin91 commented Apr 20, 2021

@li-boxuan PR is now ready to review.

@farodin91 farodin91 changed the title Setup testcontainers for hbase Setup testcontainers for hbase [tp-tests] Apr 20, 2021
@farodin91 farodin91 force-pushed the testcontainers-hbase branch from 5eded53 to 746aa1b Compare April 20, 2021 21:50
@farodin91
Copy link
Contributor Author

farodin91 commented Apr 21, 2021

I have two idea to fix the tinkerpop issues. It shouldn't be that complicated.

@farodin91 farodin91 force-pushed the testcontainers-hbase branch from 746aa1b to 7c3e795 Compare April 21, 2021 14:37
@farodin91 farodin91 added this to the Release v0.6.0 milestone Apr 21, 2021
@farodin91 farodin91 requested review from a team, FlorianHockmann and li-boxuan April 21, 2021 21:23
@farodin91
Copy link
Contributor Author

farodin91 commented Apr 22, 2021

@li-boxuan tp tests also passed. Thank you for checking it with localhost.

@farodin91 farodin91 force-pushed the testcontainers-hbase branch from a525b67 to 8ad877d Compare April 22, 2021 18:43
li-boxuan
li-boxuan reviewed Apr 23, 2021
View reviewed changes
@farodin91 farodin91 force-pushed the testcontainers-hbase branch from 8ad877d to f1c551c Compare April 23, 2021 08:45
@farodin91
Copy link
Contributor Author

farodin91 commented Apr 23, 2021

@li-boxuan All cases should be resolved.

li-boxuan
li-boxuan approved these changes Apr 23, 2021
View reviewed changes
Copy link
Member

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

Awesome! Glad to see CIs are passing again. As I said, I am not familiar with the janusgraph-hbase module, so please take my approval as a grain of salt.

porunov
porunov reviewed Apr 23, 2021
View reviewed changes
Copy link
Member

@porunov porunov left a comment

Choose a reason for hiding this comment

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

Thank you @farodin91 !
I have several small questions / nitpicks.

@farodin91 farodin91 force-pushed the testcontainers-hbase branch 2 times, most recently from d0c375d to 824e930 Compare April 23, 2021 21:57
@farodin91
Copy link
Contributor Author

farodin91 commented Apr 24, 2021

@porunov would you like Review again?

porunov
porunov approved these changes Apr 24, 2021
View reviewed changes
Copy link
Member

@porunov porunov left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @farodin91 !
I'm also not familiar with janusgraph-hbase module, so take my review with a grain of salt as well.

@porunov
Copy link
Member

porunov commented Apr 24, 2021

Looks like I introduced conflict with this PR in the previous commit. The only conflict is removing {} from the debug log:
https://github.com/porunov/janusgraph/blob/24ce75b3b28bee310b02767787fb1e7492755dd7/janusgraph-hbase-parent/janusgraph-hbase-core/src/main/java/org/janusgraph/diskstorage/hbase/HBaseStoreManager.java#L858

@farodin91
Setup testcontainers for hbase [tp-tests]
d23e952 
Fixes JanusGraph#1474

* Add docker container for hbase tests
* Merge hbase maven projects into one
* Update hbase from 2.1.5 to 2.2.7
* Update hadoop from 2.7.7 to 2.8.5

Signed-off-by: Jan Jansen <jan.jansen@gdata.de>
@farodin91 farodin91 force-pushed the testcontainers-hbase branch from 824e930 to d23e952 Compare April 24, 2021 09:21
@farodin91
Copy link
Contributor Author

farodin91 commented Apr 24, 2021

@li-boxuan @porunov I will merge after build is passed. Thank you for reviewing.

It looks like their wasn't any huge improvements in the last two year. I hope these changes and the changes i'm working on will bring back hbase to a modern state.

@farodin91 farodin91 merged commit a78b3f2 into JanusGraph:master Apr 24, 2021
@farodin91 farodin91 deleted the testcontainers-hbase branch April 24, 2021 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rngcntr rngcntr rngcntr left review comments

@porunov porunov porunov approved these changes

@li-boxuan li-boxuan li-boxuan approved these changes

@jerryjch jerryjch Awaiting requested review from jerryjch

@mbrukman mbrukman Awaiting requested review from mbrukman

@FlorianHockmann FlorianHockmann Awaiting requested review from FlorianHockmann

Assignees

No one assigned

Labels

cla: external Externally-managed CLA

Projects

None yet

Milestone

Release v0.6.0

Development

Successfully merging this pull request may close these issues.

Use testcontainers for hbase tests

5 participants

@farodin91 @li-boxuan @porunov @rngcntr @janusgraph-bot