search

JanusGraph / janusgraph-foundationdb

FoundationDB storage adapter for JanusGraph
Other
53 stars 18 forks source link

async iterator final submission #50

Closed jlazdw closed 3 years ago

jlazdw commented 3 years ago

This PR is to introduce asynchronous Iterator for getSlice And getSlices methods in FoundationDBKeyvalueStore.java ,to greatly leverage FoundationDB’s Asynchronous Iterator for on-demand data stream pulling, better memory efficiency, better parallelism for MultiQuery support. The asynchronous iterator relies on the FDB java library to take care of Completable Futures, rather than introducing additional Completable Futures in this storage plugin.

To support this new Async Iterator mode, we introduce “iterator” mode in FoundationDBConfigOptions.java. The existing getSlice and getSlices() is in the new (default) mode called “list” mode, as the query result from FDB java library gets converted to List-based result immediately.

We also provide the fix on getSlices in the current implementation.

linux-foundation-easycla[bot] commented 3 years ago

CLA Check
The committers are authorized under a signed CLA.

rngcntr commented 3 years ago

That sound like a very good improvement to me, thanks @jlazdw! I'm kindly asking @ruweih to help me out reviewing it, because he is also working on an asynchonous implementation of getSlice.

farodin91 commented 3 years ago

@jlazdw Do you want to squash all commits? Afterwards we should be able to merge it.

jlazdw commented 3 years ago

@farodin91: I have squashed all the commits up to the merge point. In between my commits, there was a PR merge to master. And thus the other commit 086b8abfb1321be241f3f68a17be85a1cfcf04f1 separated my commits. Please let me know if you like me to squash further or the current squash is OK. If it is OK, please merge the current PR to the master. I am working on improving the async iterator further (including enabling the FoundationDBGraphConcurrentTest related tests that have been disabled in current master). I will try to make another PR in the next two weeks.

farodin91 commented 3 years ago

@jlazdw In the normal case, we just want to have one commit in top of the current it possible. It would be great.

farodin91 commented 3 years ago

I am working on improving the async iterator further (including enabling the FoundationDBGraphConcurrentTest related tests that have been disabled in current master). I will try to make another PR in the next two weeks.

Sounds awesome

jerryjch commented 3 years ago

@farodin91 I think we can also use the "Squash and Merge' option to merge this PR. There is no conflict to merge.

jlazdw commented 3 years ago

@farodin91: I talked to Jerry (@jerryjch ) and he suggested above on "Squash and Merge" from your side when you merge the PR to the master. If you perform squash, could you use the following last commit message from my submission branch: https://github.com/jlazdw/janusgraph-foundationdb/commit/ec1ca28c37f019ae8c1c76aac6ba23b60be93392, which is:

" introduce asynchronous iterator unify the interface for synchronous iterator and asynchronous iterator minor clean-up on code for logging and class import and refactor record iterators (sync and async)

minor changes to the code: re-defining enum for sync and asyn iterator backend exception propagation clean-up

made changes to test environment and README.md: introduce environment-controlled option to use iterator or list mode for testing add -Dgetrangemode=iterator and -Dgetrangemode=list to Travis CI jobs *add section in README.md to explain iterator/list mode choice for GetRange related queries

"