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

enable kv interface in 2.0 #3282

Merged
merged 4 commits into from
Nov 26, 2021
Merged

enable kv interface in 2.0 #3282

merged 4 commits into from
Nov 26, 2021

Conversation

critical27
Copy link
Contributor

As title. For later kv test or jepsen, @HarrisChu @kikimo, you guys could try it. Perhaps the java client need some update for jepsen.

@HarrisChu
Copy link
Contributor

OK for me, 2 type testings:

  1. stress testing with k6, will use golang to invoke via thrift interface.
  2. jepsen

@kikimo
Copy link
Contributor

kikimo commented Nov 8, 2021

@critical27 is'it possible to add an append api?

@critical27
Copy link
Contributor Author

@critical27 is'it possible to add an append api?

What is the purpose? Could, but the performance will be very poor.

@yixinglu
Copy link
Contributor

Hi @critical27 plz solve the conflicts.

liuyu85cn
liuyu85cn previously approved these changes Nov 24, 2021
Copy link
Contributor

@liuyu85cn liuyu85cn left a comment

Choose a reason for hiding this comment

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

do we support getPrefix or removePrefix ? generally LGTM

@pengweisong
Copy link
Contributor

I think these pure kv interfaces are more suitable in separate client semantically?

Copy link
Contributor

@SuperYoko SuperYoko left a comment

Choose a reason for hiding this comment

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

LGTM. but why do we need to remove this GeneralStorageClient&Server? Maybe I need to modify my rpc modification to specific interface and keep a rpc server for those requests not from graph client, so that we can keep tools in different process.

@critical27
Copy link
Contributor Author

do we support getPrefix or removePrefix ? generally LGTM

We can't do it just like a put/get, because we need to fan out to do prefix/removePrefix from all partition, think them as a lookup. But we could do it if really necessary.

@critical27
Copy link
Contributor Author

critical27 commented Nov 25, 2021

LGTM. but why do we need to remove this GeneralStorageClient&Server? Maybe I need to modify my rpc modification to specific interface and keep a rpc server for those requests not from graph client, so that we can keep tools in different process.

The server do need to be removed, because we bind a service to a port separately, we have already used 4 ports. We definitely don't want to add a port more, the kv interface is mainly for test only.

@critical27
Copy link
Contributor Author

critical27 commented Nov 25, 2021

I think these pure kv interfaces are more suitable in separate client semantically?

Ah... The reason is we don't want to add an extra service here. (we need one more extra port to bind), so both the service and client will be removed. In 1.0, they are in the same service as well.

@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2021

Codecov Report

Merging #3282 (033a6a2) into master (6541438) will decrease coverage by 0.09%.
The diff coverage is 80.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3282      +/-   ##
==========================================
- Coverage   85.31%   85.22%   -0.10%     
==========================================
  Files        1289     1276      -13     
  Lines      120073   118982    -1091     
==========================================
- Hits       102442   101398    -1044     
+ Misses      17631    17584      -47     
Impacted Files Coverage Δ
src/clients/meta/MetaClient.cpp 75.62% <ø> (-0.90%) ⬇️
src/clients/meta/MetaClient.h 95.65% <ø> (ø)
src/graph/context/QueryContext.cpp 100.00% <ø> (ø)
src/graph/executor/mutate/DeleteExecutor.cpp 83.22% <ø> (ø)
src/graph/executor/mutate/InsertExecutor.cpp 100.00% <ø> (ø)
src/graph/executor/mutate/UpdateExecutor.cpp 87.64% <ø> (ø)
src/graph/executor/query/IndexScanExecutor.h 100.00% <ø> (ø)
src/graph/executor/query/TraverseExecutor.h 100.00% <ø> (ø)
src/graph/optimizer/rule/TopNRule.cpp 95.83% <ø> (ø)
src/graph/planner/plan/Admin.cpp 56.14% <0.00%> (-2.05%) ⬇️
... and 98 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a14d7b4...033a6a2. Read the comment docs.

Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@panda-sheep panda-sheep left a comment

Choose a reason for hiding this comment

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

Good job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants