[kudu-CR] Implement an upgrade test

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Implement an upgrade test
..


Patch Set 2:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/4424/2//COMMIT_MSG
Commit Message:

PS2, Line 7: upgrade
> Wouldn't this also be a downgrade test if we happen to use the binary-set B
Done


PS2, Line 9: a simple
> This is bit more white-boxy than I initially thought. Thinking out loud her
I don't think there are currently any integration tests that could be 
repurposed in the way you are describing. If that is true, then we have to 
write new tests.

This first test is very "white box" but that doesn't mean we can't easily write 
more "black box" tests using the existing tools / libraries we already use for 
integration testing in the rest of the code base.


http://gerrit.cloudera.org:8080/#/c/4424/2/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

Line 255:   void SetDaemonBinPath(std::string daemon_bin_path);
> Nit: name it to SetBinaryPath in comparison to GetBinaryPath below?
I don't think that would make sense. GetBinaryPath() takes the name of a binary 
to resolve into an absolute path based on what the daemon_bin_path was set to.


http://gerrit.cloudera.org:8080/#/c/4424/2/src/kudu/integration-tests/upgrade-test.cc
File src/kudu/integration-tests/upgrade-test.cc:

PS2, Line 18: #include 
: #include 
> nit: consider listing these after STL includes, as recommended by the style
Done


Line 22: #include 
> nit: is this really needed for the code below?
Done


Line 38: DEFINE_int32(num_snapshots, 3, "Number of snapshots to verify across 
replicas and reboots.");
> nit: s/num_snapshots/num_samples ?
I just copy / pasted this from linked_list-test so I think it's reasonable to 
keep the flags named the same.


Line 41: using std::unique_ptr;
> consider adding
Done


PS2, Line 53: if (FLAGS_test_version_a_bin.empty() || 
FLAGS_test_version_b_bin.empty()) {
:   LOG(FATAL) << "Must specify --test_version_a_bin and 
--test_version_b_bin flags";
: }
> nit: does it make  sense to check for this first before running KuduTest::S
Done


PS2, Line 58: std::vector()
> nit: you could use c++11 features to shorten the signature, i.e. use '{}' i
cool, never done that for default args


PS2, Line 76: undefok
> nit: do we need undefok here and below ?
Yes, we need it because those flags are not in older versions of kudu-tserver 
and kudu-master, but the ExternalMiniCluster passes them into the binary when 
invoking them.


Line 104: 
> Do you think it's worth verifying that the server with the same version is 
Yes, that's already being done with the call to WaitAndVerify()


PS2, Line 108: FLAGS_test_version_a_bin
> Typo: FLAGS_test_version_b_bin
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2831b47e6c0b644a256e1914fa495f453318e0cd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] Implement an upgrade test

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: Implement an upgrade test
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4424/2/src/kudu/integration-tests/upgrade-test.cc
File src/kudu/integration-tests/upgrade-test.cc:

PS2, Line 108: FLAGS_test_version_a_bin
Typo: FLAGS_test_version_b_bin


-- 
To view, visit http://gerrit.cloudera.org:8080/4424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2831b47e6c0b644a256e1914fa495f453318e0cd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] Implement an upgrade test

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4424

to look at the new patch set (#2).

Change subject: Implement an upgrade test
..

Implement an upgrade test

This program takes two binary directories and runs a simple "upgrade"
test between the two versions to ensure that a table created with
"Version A" of the software can be read by "Version B" of the software.

This patch also makes some changes to ExternalMiniCluster and the cmake
files to allow for creating an executable that is written like a unit
test (using the Google Test framework) but is not automatically executed
by ctest.

As a follow-up, we would benefit from a higher-level script that can
manage upgrade and downgrade testing across many versions of Kudu.

Change-Id: I2831b47e6c0b644a256e1914fa495f453318e0cd
---
M CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/upgrade-test.cc
5 files changed, 158 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/4424/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2831b47e6c0b644a256e1914fa495f453318e0cd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot