You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/12/10 03:07:13 UTC
[kudu-CR] tests: set never fsync for every test
Hello Dan Burkert, Mike Percy,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5459
to review the following change.
Change subject: tests: set never_fsync for every test
......................................................................
tests: set never_fsync for every test
None of our tests rely on unplugging machines, forcefully unmounting a
filesystem, or any other equivalent activity where fsync()'s durability
guarantees might be necessary. Moreover, we were already disabling fsync()
in a bunch of tests; this patch just centralizes it into KuduTest so all
tests get it for free. Doing so means tests wil run faster and we can remove
some duplicated test code.
Some tests used enable_data_block_fsync=false instead. I think it predates
never_fsync, and it's not nearly as good of a choice, as it leads to
slightly different code paths taken (and thus different coverage) in the
block managers.
I don't think the change to compaction-test will regress the bug fixed by
commit 7d92ed6; other tests have since begun using
Env::GetFileSizeOnDiskRecursively() without issue, and I don't think our
test environments are so messed up that they're mounting ext4 as ext3.
I ran ctest serially on a debug build (fast tests) and here's what I got:
Vanilla
-------
Total Test time (real) = 957.84 sec
real 15m57.866s
user 13m47.340s
sys 2m24.148s
Modified
--------
Total Test time (real) = 816.72 sec
real 13m36.820s
user 14m34.812s
sys 2m27.704s
So a pretty nice improvement already.
Change-Id: Ia8c2574eb3cb76fba1bce0202d4335d8b7035a93
---
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/client-test.cc
M src/kudu/common/encoded_key-test.cc
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/consistency-itest.cc
M src/kudu/integration-tests/create-table-itest.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/integration-tests/version_migration-test.cc
M src/kudu/integration-tests/webserver-stress-itest.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/util/atomic-test.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flags-test.cc
M src/kudu/util/test_util.cc
30 files changed, 48 insertions(+), 110 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/5459/1
--
To view, visit http://gerrit.cloudera.org:8080/5459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia8c2574eb3cb76fba1bce0202d4335d8b7035a93
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] tests: set never fsync for every test
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: tests: set never_fsync for every test
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5459/1/src/kudu/util/atomic-test.cc
File src/kudu/util/atomic-test.cc:
Line 23: #include <gtest/gtest.h>
> Depends. If you're a believe in strict include-what-you-use, then every "le
In this case, the superclass must inherit from gtest, so we know it isn't required. But I'm not very worried about it either way.
--
To view, visit http://gerrit.cloudera.org:8080/5459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c2574eb3cb76fba1bce0202d4335d8b7035a93
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] tests: set never fsync for every test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: tests: set never_fsync for every test
......................................................................
tests: set never_fsync for every test
None of our tests rely on unplugging machines, forcefully unmounting a
filesystem, or any other equivalent activity where fsync()'s durability
guarantees might be necessary. Moreover, we were already disabling fsync()
in a bunch of tests; this patch just centralizes it into KuduTest so all
tests get it for free. Doing so means tests wil run faster and we can remove
some duplicated test code.
Some tests used enable_data_block_fsync=false instead. I think it predates
never_fsync, and it's not nearly as good of a choice, as it leads to
slightly different code paths taken (and thus different coverage) in the
block managers.
I don't think the change to compaction-test will regress the bug fixed by
commit 7d92ed6; other tests have since begun using
Env::GetFileSizeOnDiskRecursively() without issue, and I don't think our
test environments are so messed up that they're mounting ext4 as ext3.
I ran ctest serially on a debug build (fast tests) and here's what I got:
Vanilla
-------
Total Test time (real) = 957.84 sec
real 15m57.866s
user 13m47.340s
sys 2m24.148s
Modified
--------
Total Test time (real) = 816.72 sec
real 13m36.820s
user 14m34.812s
sys 2m27.704s
So a pretty nice improvement already.
Change-Id: Ia8c2574eb3cb76fba1bce0202d4335d8b7035a93
Reviewed-on: http://gerrit.cloudera.org:8080/5459
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/client-test.cc
M src/kudu/common/encoded_key-test.cc
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/consistency-itest.cc
M src/kudu/integration-tests/create-table-itest.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/integration-tests/version_migration-test.cc
M src/kudu/integration-tests/webserver-stress-itest.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/util/atomic-test.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flags-test.cc
M src/kudu/util/test_util.cc
29 files changed, 49 insertions(+), 111 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia8c2574eb3cb76fba1bce0202d4335d8b7035a93
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tests: set never fsync for every test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5459
to look at the new patch set (#2).
Change subject: tests: set never_fsync for every test
......................................................................
tests: set never_fsync for every test
None of our tests rely on unplugging machines, forcefully unmounting a
filesystem, or any other equivalent activity where fsync()'s durability
guarantees might be necessary. Moreover, we were already disabling fsync()
in a bunch of tests; this patch just centralizes it into KuduTest so all
tests get it for free. Doing so means tests wil run faster and we can remove
some duplicated test code.
Some tests used enable_data_block_fsync=false instead. I think it predates
never_fsync, and it's not nearly as good of a choice, as it leads to
slightly different code paths taken (and thus different coverage) in the
block managers.
I don't think the change to compaction-test will regress the bug fixed by
commit 7d92ed6; other tests have since begun using
Env::GetFileSizeOnDiskRecursively() without issue, and I don't think our
test environments are so messed up that they're mounting ext4 as ext3.
I ran ctest serially on a debug build (fast tests) and here's what I got:
Vanilla
-------
Total Test time (real) = 957.84 sec
real 15m57.866s
user 13m47.340s
sys 2m24.148s
Modified
--------
Total Test time (real) = 816.72 sec
real 13m36.820s
user 14m34.812s
sys 2m27.704s
So a pretty nice improvement already.
Change-Id: Ia8c2574eb3cb76fba1bce0202d4335d8b7035a93
---
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/client-test.cc
M src/kudu/common/encoded_key-test.cc
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/consistency-itest.cc
M src/kudu/integration-tests/create-table-itest.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/integration-tests/version_migration-test.cc
M src/kudu/integration-tests/webserver-stress-itest.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/util/atomic-test.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flags-test.cc
M src/kudu/util/test_util.cc
29 files changed, 49 insertions(+), 111 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/5459/2
--
To view, visit http://gerrit.cloudera.org:8080/5459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c2574eb3cb76fba1bce0202d4335d8b7035a93
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tests: set never fsync for every test
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: tests: set never_fsync for every test
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5459/1/src/kudu/util/atomic-test.cc
File src/kudu/util/atomic-test.cc:
Line 23: #include <gtest/gtest.h>
nit: Superfluous when pulling in test_util.h
--
To view, visit http://gerrit.cloudera.org:8080/5459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c2574eb3cb76fba1bce0202d4335d8b7035a93
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] tests: set never fsync for every test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: tests: set never_fsync for every test
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5459/1/src/kudu/util/atomic-test.cc
File src/kudu/util/atomic-test.cc:
Line 23: #include <gtest/gtest.h>
> nit: Superfluous when pulling in test_util.h
Depends. If you're a believe in strict include-what-you-use, then every "leaf" dependency ought to include every header it uses, even if that header is included via intermediate header (imagine the case where the intermediate header loses that dependency; now a bunch of leaf dependencies don't build).
--
To view, visit http://gerrit.cloudera.org:8080/5459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c2574eb3cb76fba1bce0202d4335d8b7035a93
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] tests: set never fsync for every test
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: tests: set never_fsync for every test
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/5459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c2574eb3cb76fba1bce0202d4335d8b7035a93
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] tests: set never fsync for every test
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: tests: set never_fsync for every test
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c2574eb3cb76fba1bce0202d4335d8b7035a93
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No