Mike Percy has posted comments on this change.

Change subject: Implement an upgrade test

Patch Set 2:


Commit Message:

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

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.

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.

File src/kudu/integration-tests/upgrade-test.cc:

PS2, Line 18: #include <gflags/gflags.h>
            : #include <gtest/gtest.h>
> nit: consider listing these after STL includes, as recommended by the style

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

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

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

PS2, Line 58: std::vector<std::string>()
> 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

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 <mpe...@apache.org>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-HasComments: Yes

Reply via email to