Alexey Serbin has posted comments on this change. Change subject: KUDU-871. Support tombstoned voting ......................................................................
Patch Set 10: (12 comments) http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/integration-tests/cluster_itest_util.h File src/kudu/integration-tests/cluster_itest_util.h: PS10, Line 236: // Request the given replica to vote. nit: could you document at least first 5 parameters of this method? http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/integration-tests/tombstoned_voting-itest.cc File src/kudu/integration-tests/tombstoned_voting-itest.cc: Line 18: #include "kudu/consensus/metadata.pb.h" nit: from IWYU src/kudu/integration-tests/tombstoned_voting-itest.cc should add these lines: #include <gflags/gflags_declare.h> #include <glog/logging.h> #include <gtest/gtest.h> #include <boost/optional/optional.hpp> #include <cstdint> #include <memory> #include <ostream> #include <string> #include <unordered_map> #include <vector> #include "kudu/consensus/consensus.pb.h" #include "kudu/consensus/opid.pb.h" #include "kudu/consensus/opid_util.h" #include "kudu/fs/fs_manager.h" #include "kudu/gutil/ref_counted.h" #include "kudu/integration-tests/internal_mini_cluster.h" #include "kudu/tablet/metadata.pb.h" #include "kudu/tserver/tserver.pb.h" #include "kudu/util/env.h" #include "kudu/util/monotime.h" #include "kudu/util/status.h" #include "kudu/util/test_macros.h" #include "kudu/util/test_util.h" PS10, Line 167: TS1 nit: the tablet? Or it's possible to tombstone a tablet server now? PS10, Line 172: TServerDetails* ts1_ets nit: consider moving this closer to the call site where it's used. Line 173: scoped_refptr<TabletReplica> replica; nit: does it make sense to verify the scenario below works without errors even after restarting the tablet server TS1? PS10, Line 181: "A" Could you add a comment explaining that the actual candidate UUID is not crucial in this test? PS10, Line 249: ts0_replica->consensus()->StepDown(&resp) nit: should it be some selective error handling based on the code set in the 'resp'? What if it failed due to some other reason -- would the test handle that properly? PS10, Line 253: ts0 nit: 'TS0' since it has been referred as that already. PS10, Line 256: FOLLOWER Could it change to something like 'LEARNER' in the middle and bring some flakiness? http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/integration-tests/tombstoned_voting-stress-test.cc File src/kudu/integration-tests/tombstoned_voting-stress-test.cc: Line 18: #include <atomic> nit: from IWYU report: src/kudu/integration-tests/tombstoned_voting-stress-test.cc should add these lines: #include <glog/logging.h> #include <gtest/gtest.h> #include <boost/optional/optional.hpp> #include <cstdint> #include <memory> #include <ostream> #include <string> #include <unordered_map> #include <vector> #include "kudu/common/wire_protocol.pb.h" #include "kudu/consensus/consensus.pb.h" #include "kudu/consensus/opid.pb.h" #include "kudu/gutil/macros.h" #include "kudu/integration-tests/external_mini_cluster.h" #include "kudu/integration-tests/external_mini_cluster_fs_inspector.h" #include "kudu/tablet/metadata.pb.h" #include "kudu/util/monotime.h" #include "kudu/util/net/net_util.h" #include "kudu/util/status.h" #include "kudu/util/test_macros.h" #include "kudu/util/test_util.h" src/kudu/integration-tests/tombstoned_voting-stress-test.cc should remove these lines: - #include "kudu/consensus/metadata.pb.h" // lines 25-25 - #include "kudu/consensus/raft_consensus.h" // lines 26-26 - #include "kudu/util/random.h" // lines 32-32 http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 2489: LOG(WARNING) << "Tablet " << tablet->ToString() << " has failed on TS " wouldn't it be natural to have tablet::SHUTDOWN check here instead? Or it's not guaranteed to be SHUTDOWN at this point? http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: PS10, Line 304: LOG(DFATAL) The rest of the CHECKs would work for non-debug builds as well. What is the reason of having LOG(DFATAL), not LOG(FATAL) here? If there is any, please add a comment. -- To view, visit http://gerrit.cloudera.org:8080/6960 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia19d75b185299443b27f41e468bbae20065e7570 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Edward Fancher <e...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
