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

[to #375] use scanRegions request to warm up client #383

Merged
merged 11 commits into from
Dec 24, 2021

Conversation

birdstorm
Copy link
Collaborator

@birdstorm birdstorm commented Dec 9, 2021

use scanRegions request with limit to warm up client.

new configurations:

tikv.warm_up.enable default=true
tikv.grpc.warm_up_timeout_in_ms default=5000ms

Signed-off-by: birdstorm <samuelwyf@hotmail.com>
@zz-jason zz-jason changed the title Use scanRegions request to warm up client [to #375] use scanRegions request to warm up client Dec 9, 2021
birdstorm and others added 4 commits December 10, 2021 04:25
Signed-off-by: birdstorm <samuelwyf@hotmail.com>
…storm/client-java into use-scan-regions-request-to-warm-up
Signed-off-by: birdstorm <samuelwyf@hotmail.com>
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

could you add some test for this PR?

Signed-off-by: birdstorm <samuelwyf@hotmail.com>
Signed-off-by: birdstorm <samuelwyf@hotmail.com>
Signed-off-by: birdstorm <samuelwyf@hotmail.com>

private void doTest(TiConfiguration conf) throws Exception {
session = TiSession.create(conf);
long start = System.currentTimeMillis();
Copy link
Member

Choose a reason for hiding this comment

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

is it better to use System.nanoTime()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it is not necessary in unit test.

Comment on lines 133 to 135
doTest(conf);
conf.setWarmUpEnable(false);
doTest(conf);
Copy link
Member

Choose a reason for hiding this comment

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

how about:

elapsedWithWarmupMS  = doTest(conf)
conf.setWarmUpEnable(false);
elapsedWithoutWarmupMS = doTest(conf);
assertTrue(elapsedWithWarmupMS < elapsedWithoutWarmupMS)

Copy link
Member

Choose a reason for hiding this comment

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

@birdstorm PTAL at this comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@zz-jason PTAL

Copy link
Collaborator

@marsishandsome marsishandsome left a comment

Choose a reason for hiding this comment

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

LGTM

@marsishandsome
Copy link
Collaborator

/run-all-tests

Signed-off-by: birdstorm <samuelwyf@hotmail.com>
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Collaborator

/run-all-tests

@zz-jason zz-jason merged commit 468e999 into tikv:master Dec 24, 2021
ti-srebot pushed a commit to ti-srebot/client-java that referenced this pull request Dec 24, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Collaborator

cherry pick to release-3.1 in PR #438

@birdstorm birdstorm deleted the use-scan-regions-request-to-warm-up branch December 24, 2021 04:39
birdstorm added a commit to ti-srebot/client-java that referenced this pull request Dec 24, 2021
birdstorm added a commit to ti-srebot/client-java that referenced this pull request Dec 28, 2021
Signed-off-by: birdstorm <samuelwyf@hotmail.com>
birdstorm added a commit to ti-srebot/client-java that referenced this pull request Dec 28, 2021
Signed-off-by: birdstorm <samuelwyf@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants