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 <gflags/gflags.h> : #include <gtest/gtest.h> > nit: consider listing these after STL includes, as recommended by the style Done Line 22: #include <unordered_map> > 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<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 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 <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
