-
Notifications
You must be signed in to change notification settings - Fork 110
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
RawKV support SST Ingest using KVStream interface #233
RawKV support SST Ingest using KVStream interface #233
Conversation
fd126c5
to
e03b641
Compare
a43e7e2
to
6ec7e09
Compare
/run-all-tests tikv=pr/10524 |
1 similar comment
/run-all-tests tikv=pr/10524 |
fca1963
to
3dc1da2
Compare
3dc1da2
to
7b47468
Compare
7b47468
to
a24b2ed
Compare
return splitRegion(splitKeys, backOffer, 1); | ||
} | ||
|
||
private List<Metapb.Region> splitRegion( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use recursive function to retry rather than a loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a simple retry, e.g.
- we need to split 6 keys
- 2 keys are split successfully, 4 keys are failed
- retry to split the 4 failed keys
It's easy to use a recursive function to implement this logic.
public void keepTiKVToImportMode() { | ||
ingestScheduledExecutorService.scheduleAtFixedRate( | ||
this::switchTiKVToImportMode, 0, KEEP_TIKV_TO_IMPORT_MODE_PERIOD, TimeUnit.SECONDS); | ||
} | ||
|
||
public void stopKeepTiKVToImportMode() { | ||
ingestScheduledExecutorService.shutdown(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to keep this two function private and be called automatically by switchTiKVToXXXMode()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switchTiKVToNormalMode
is called one time, while keepTiKVToImportMode
will start a scheduler and call switchTiKVToImportMode
every 2 minute.
The name keepTiKVToImportMode
is used in order that
- let users know that a scheduler will be started after calling
keepTiKVToImportMode
stopKeepTiKVToImportMode
must be called to stop the scheduler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to call keep import repeatedly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I presumed that there will be multiple import clients. But now I know that the API is for example purpose.
returnNumber = 0; | ||
for (ImporterStoreClient client : clientList) { | ||
if (client.isRawWriteResponseReceived()) { | ||
returnNumber++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the client is not removed from clientList
, will it count twice in the next iteration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactor this code, PTAL
|
||
private void ingest() throws GrpcException { | ||
int returnNumber = 0; | ||
while (returnNumber < clientList.size()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this dead loop if a client never succeed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then will go to client.hasRawWriteResponseError()
and throw new GrpcException
src/main/java/org/tikv/common/importer/ImporterStoreClient.java
Outdated
Show resolved
Hide resolved
bdba26f
to
40686fc
Compare
@andylokandy PTAL |
Signed-off-by: marsishandsome <[email protected]>
Signed-off-by: marsishandsome <[email protected]>
Signed-off-by: marsishandsome <[email protected]>
Signed-off-by: marsishandsome <[email protected]>
356fa69
to
eecb406
Compare
Signed-off-by: marsishandsome <[email protected]>
eecb406
to
4945902
Compare
Signed-off-by: marsishandsome <[email protected]>
/run-all-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What problem does this PR solve?
This is a subtask of tikv/tikv#10563.
What is changed and how it works?
TiKV java client support Bulk Load using the ingest API.
this PR depends on:
Check List
Tests
Code changes
Side effects
Related changes