You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/01/26 22:05:58 UTC
[kudu-CR] Reduce default RSA key length for tests
Hello Dan Burkert, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5804
to review the following change.
Change subject: Reduce default RSA key length for tests
......................................................................
Reduce default RSA key length for tests
The default RSA key length for real servers is set to 2048 bits.
However, generating a 2048-bit RSA key is relatively expensive. Since we
do it on every server start, and our tests start thousands of servers,
this has a negative impact on test time.
This changes the default in ExternalMiniCluster and in our own tests to
512-bit keys, which appear to be about 16x faster to generate compared
to 2048-bit keys (based on 'perf stat -r50 openssl genrsa -out /tmp/r
<key length>').
I measured the improvement on registration-test from ~4000ms to ~1200ms
wall time on my laptop.
This also changes the way we override flags for tests to use the gflags
API instead of directly setting FLAGS_* variables. This cleans up some
workarounds we did previously and also allows us to set flags which
might not apply for all test cases (eg security flags aren't in util
tests).
Change-Id: I310ac762aff4fed891a0491b2c3249e0faa9375f
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flags-test.cc
M src/kudu/util/test_util.cc
4 files changed, 27 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/5804/1
--
To view, visit http://gerrit.cloudera.org:8080/5804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I310ac762aff4fed891a0491b2c3249e0faa9375f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
[kudu-CR] Reduce default RSA key length for tests
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Reduce default RSA key length for tests
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5804/1/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
PS1, Line 79: // We don't check for errors here, because we have some default flags that
: // only apply to certain tests.
> What if we called GetCommandLineFlagInfoOrDie() on each one beforehand, to
Exactly the latter - that's the reason for this change in the first place. eg any test in util doesn't depend on 'security' and thus won't have those flags. We only got lucky before in that the original two flags were in util
--
To view, visit http://gerrit.cloudera.org:8080/5804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I310ac762aff4fed891a0491b2c3249e0faa9375f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Reduce default RSA key length for tests
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: Reduce default RSA key length for tests
......................................................................
Reduce default RSA key length for tests
The default RSA key length for real servers is set to 2048 bits.
However, generating a 2048-bit RSA key is relatively expensive. Since we
do it on every server start, and our tests start thousands of servers,
this has a negative impact on test time.
This changes the default in ExternalMiniCluster and in our own tests to
512-bit keys, which appear to be about 16x faster to generate compared
to 2048-bit keys (based on 'perf stat -r50 openssl genrsa -out /tmp/r
<key length>').
I measured the improvement on registration-test from ~4000ms to ~1200ms
wall time on my laptop.
This also changes the way we override flags for tests to use the gflags
API instead of directly setting FLAGS_* variables. This cleans up some
workarounds we did previously and also allows us to set flags which
might not apply for all test cases (eg security flags aren't in util
tests).
Change-Id: I310ac762aff4fed891a0491b2c3249e0faa9375f
Reviewed-on: http://gerrit.cloudera.org:8080/5804
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flags-test.cc
M src/kudu/util/test_util.cc
4 files changed, 27 insertions(+), 27 deletions(-)
Approvals:
Dan Burkert: Looks good to me, but someone else must approve
Adar Dembo: Looks good to me, approved
Alexey Serbin: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I310ac762aff4fed891a0491b2c3249e0faa9375f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Reduce default RSA key length for tests
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Reduce default RSA key length for tests
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/5804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I310ac762aff4fed891a0491b2c3249e0faa9375f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Reduce default RSA key length for tests
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Reduce default RSA key length for tests
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5804/1/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
PS1, Line 79: // We don't check for errors here, because we have some default flags that
: // only apply to certain tests.
> yea, but there's not really a better way of doing it that won't take signif
What if we called GetCommandLineFlagInfoOrDie() on each one beforehand, to verify that it exists? Or does that not work because the rsa key flags aren't linked into every test?
--
To view, visit http://gerrit.cloudera.org:8080/5804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I310ac762aff4fed891a0491b2c3249e0faa9375f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Reduce default RSA key length for tests
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Reduce default RSA key length for tests
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5804/1//COMMIT_MSG
Commit Message:
PS1, Line 22: This also changes the way we override flags for tests to use the gflags
: API instead of directly setting FLAGS_* variables. This cleans up some
: workarounds we did previously and also allows us to set flags which
: might not apply for all test cases (eg security flags aren't in util
: tests).
> nit: consider putting this part of the patch into a separate changelist.
It has to be done in order to set the RSA length, because the RSA length flag is defined outside of util
http://gerrit.cloudera.org:8080/#/c/5804/1/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
PS1, Line 79: // We don't check for errors here, because we have some default flags that
: // only apply to certain tests.
> Hmm, but doesn't that mean that if one of these flags is renamed or removed
yea, but there's not really a better way of doing it that won't take significant coding... and if we dropped one of these, the worst repercussion would be our tests would get slower again and we'd probably notice it?
--
To view, visit http://gerrit.cloudera.org:8080/5804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I310ac762aff4fed891a0491b2c3249e0faa9375f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Reduce default RSA key length for tests
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Reduce default RSA key length for tests
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5804/1/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
PS1, Line 79: // We don't check for errors here, because we have some default flags that
: // only apply to certain tests.
Hmm, but doesn't that mean that if one of these flags is renamed or removed, we'll never know to update this code? Seems risky.
--
To view, visit http://gerrit.cloudera.org:8080/5804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I310ac762aff4fed891a0491b2c3249e0faa9375f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] Reduce default RSA key length for tests
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Reduce default RSA key length for tests
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5804/1/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
PS1, Line 79: // We don't check for errors here, because we have some default flags that
: // only apply to certain tests.
> Exactly the latter - that's the reason for this change in the first place.
We could note for each key whether it's universal or not, and only verify the existence of the universal ones. But, that doesn't buy us much.
Alright. Uncle.
--
To view, visit http://gerrit.cloudera.org:8080/5804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I310ac762aff4fed891a0491b2c3249e0faa9375f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Reduce default RSA key length for tests
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Reduce default RSA key length for tests
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5804/1//COMMIT_MSG
Commit Message:
PS1, Line 22: This also changes the way we override flags for tests to use the gflags
: API instead of directly setting FLAGS_* variables. This cleans up some
: workarounds we did previously and also allows us to set flags which
: might not apply for all test cases (eg security flags aren't in util
: tests).
nit: consider putting this part of the patch into a separate changelist.
--
To view, visit http://gerrit.cloudera.org:8080/5804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I310ac762aff4fed891a0491b2c3249e0faa9375f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes