Skip to content
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.

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

[hbasestore] HBase-powered KVStore implementation #413

Merged
merged 4 commits into from Jun 26, 2019

Conversation

@zhangguoqing
Copy link
Collaborator

commented May 22, 2019

close #179

@nebula-community-bot

This comment has been minimized.

Copy link

commented May 22, 2019

Can one of the admins verify this patch?

@dangleptr dangleptr requested review from dangleptr and sherman-the-tank May 22, 2019

@zhangguoqing

This comment has been minimized.

Copy link
Collaborator Author

commented May 22, 2019

TODO:

  • 1. use the NebulaKeyUtils #402 to make the storage pass by modifying CMakeLists.txt only.
  • 2. use a test runner to set up an inside hbase testbed.
@dangleptr

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

TODO:

  1. use the NebulaKeyUtils #402 to make the storage pass by modifying CMakeLists.txt only.
  2. use a test runner to set up a inside hbase testbed.

Excellent!
How do you run your UTs currently?

@zhangguoqing

This comment has been minimized.

Copy link
Collaborator Author

commented May 22, 2019

Currently, a standalone HBase environment with local file mode was started on my development machine. And all kvstore test cases are pass. :)
https://github.com/vesoft-inc/nebula/pull/413/files#diff-9d431e623cca5bcbd11ca24b91c18bb2R13

@dangleptr

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Currently, a standalone HBase environment with local file mode was started on my development machine. And all kvstore test cases are pass. :)

That's cool. Do you want to implement the runner you mentioned?

@zhangguoqing

This comment has been minimized.

Copy link
Collaborator Author

commented May 22, 2019

@sherman-the-tank said before that he would implement this runner, and it seems that he is very experienced in this case. What I did might not meet his expectations, awkward ^_^|||.

@dangleptr

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

awkward

Ha Ha... That's fine!
@sherman-the-tank is too busy recently, if you want to have a try, please go ahead.
We could discuss how to do it.

@zhangguoqing

This comment has been minimized.

Copy link
Collaborator Author

commented May 22, 2019

Thx. But now, I want to take one new case with graph and parser modules. That will help me understand Nebula in a more comprehensive and holistic manner. ;-)

@dangleptr

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Thx. But now, I want to take one new case with graph and parser modules. That will help me understand Nebula in a more comprehensive and holistic manner. ;-)

That's fine. Let's disable the UTs firstly before hbase runner merged in. Let me take care of it.

@dangleptr

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

After skim the pr, i think we need to refactor the kvstore module.
Maybe the structure looks like following:

kvstore
 |--------- kvstore.h
 |----------inhouse / ...   
 |----------hbase   / ...
tableName_ = kHBaseTableNamePrefix + folly::to<std::string>(spaceId_);
partsNum_ = this->allParts().size();
// TODO(zhangguoqing) check/create the hbase table named tableName_.
// Now, the version 1.4.9 of HBase thrift2 does not support APIs for table operations.

This comment has been minimized.

Copy link
@darionyaphet

darionyaphet May 22, 2019

Contributor

why HBase 1.4.9 ? The latest one is 2.1.4

This comment has been minimized.

Copy link
@zhangguoqing

zhangguoqing May 22, 2019

Author Collaborator

In my opinion, if we really want to use this feature in a real production environment, we should be most compatible with the widely used version rather than the latest version 2.1.4. As far as I know, hbase version 1.4.9 is widely used, and this version of thrift2 is also forward compatible with rel/1.4.0, and which is not substantially different from rel/2.1.4 [1]. The APIs for table operation is support after the branch-2.2 which is not released now.

[1] diff hbase.thrift149 hbase.thrift214
78c78, 80
< DELETE_COLUMNS = 1
---
> DELETE_COLUMNS = 1,
> DELETE_FAMILY = 2,
> DELETE_FAMILY_VERSION = 3
210a213,218
> enum TReadType {
> DEFAULT = 1,
> STREAM = 2,
> PREAD = 3
> }
>
229c237,238
< 14: optional bool small
---
> 14: optional TReadType readType
> 15: optional i32 limit

This comment has been minimized.

Copy link
@darionyaphet

darionyaphet May 22, 2019

Contributor

Understood. According to your statement, we should create table from shell before using it .

This comment has been minimized.

Copy link
@zhangguoqing

zhangguoqing May 22, 2019

Author Collaborator

Yes, for unit tests, these operations[1] will be done by test runner.

[1] some operations:

  • Add a test runner to provide HBase/thrift2 service.
  • hbase/bin/hbase-daemon.sh start thrift2 -b 127.0.0.1 -p 9096
  • hbase(main):001:0> create 'Nebula_Graph_Space_0', 'cf'
@zhangguoqing

This comment has been minimized.

Copy link
Collaborator Author

commented May 22, 2019

After skim the pr, i think we need to refactor the kvstore module.
Maybe the structure looks like following:

kvstore
 |--------- kvstore.h
 |----------inhouse / ...   
 |----------hbase   / ...

Now, I will just extract hbase as a separate module. FYI. #344

@zhangguoqing zhangguoqing force-pushed the zhangguoqing:hbasestore branch from c7056a8 to e77819e May 22, 2019

src/kvstore/hbase/HBaseStore.h Outdated Show resolved Hide resolved
src/kvstore/CMakeLists.txt Outdated Show resolved Hide resolved
src/kvstore/test/CMakeLists.txt Outdated Show resolved Hide resolved

@zhangguoqing zhangguoqing force-pushed the zhangguoqing:hbasestore branch from e77819e to 9f69a42 May 24, 2019

@dangleptr
Copy link
Contributor

left a comment

The code structure looks better now.

src/kvstore/plugins/hbase/HBaseStore.h Outdated Show resolved Hide resolved
src/kvstore/plugins/hbase/HBaseStore.h Outdated Show resolved Hide resolved
src/kvstore/test/CMakeLists.txt Outdated Show resolved Hide resolved
@dangleptr

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

By the way, you should disable all UTs before runner merged in.

@zhangguoqing zhangguoqing force-pushed the zhangguoqing:hbasestore branch from 9f69a42 to e116692 May 28, 2019

@dangleptr

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Close #179

@zhangguoqing zhangguoqing force-pushed the zhangguoqing:hbasestore branch 3 times, most recently from 6231dba to b44fcc9 Jun 13, 2019

@dangleptr

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

The pr looks better now.
Jenkins go

@dangleptr

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

Jenkins go

1 similar comment
@dangleptr

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

Jenkins go

@dangleptr

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

Jenkins go

@nebula-community-bot

This comment has been minimized.

Copy link

commented Jun 17, 2019

Unit testing failed.

zhangguoqing added some commits Apr 19, 2019

[hbasestore] Refactor the hbasestore as a separate module.
1. replace storage/KeyUtils with base/NebulaKeyUtils
2. put the hbase.thrift to kvstore/hbase
3. address current all comments
[hbasestore] Move hbase to plugins and disable the 'Partition'
NOTE: Temporary disable the hbase related unit tests hbase_*_test in
src/kvstore/plugins/hbase/test/CMakeLists.txt.

@zhangguoqing zhangguoqing force-pushed the zhangguoqing:hbasestore branch from b44fcc9 to 318c19e Jun 18, 2019

@dangleptr

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

Jenkins go

@nebula-community-bot

This comment has been minimized.

Copy link

commented Jun 20, 2019

Unit testing failed.

@dangleptr

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Jenkins go

@nebula-community-bot

This comment has been minimized.

Copy link

commented Jun 21, 2019

Unit testing failed.

@dangleptr

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

Jenkins go

@nebula-community-bot

This comment has been minimized.

Copy link

commented Jun 26, 2019

Unit testing passed.

@dangleptr
Copy link
Contributor

left a comment

Well done!
Thanks for your great work! @zhangguoqing

@darionyaphet
Copy link
Contributor

left a comment

LGTM 🎉

@dangleptr dangleptr merged commit e15c065 into vesoft-inc:master Jun 26, 2019

1 check passed

UnitTest All tests passed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.