Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11428 )
Change subject: KUDU-2463 pt 3: don't scan if MVCC hasn't moved ...................................................................... Patch Set 2: (7 comments) Looks good to me http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/integration-tests/timestamp_advancement-itest.cc File src/kudu/integration-tests/timestamp_advancement-itest.cc: http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/integration-tests/timestamp_advancement-itest.cc@295 PS2, Line 295: ScanResponsePB resp; : RpcController rpc; : ScanRequestPB req = ReqForTablet(tablet_id); : shared_ptr<TabletServerServiceProxy> tserver_proxy = cluster_->tserver_proxy(0); : ASSERT_OK(tserver_proxy->Scan(req, &resp, &rpc)); : LOG(INFO) << "Got response: " << SecureShortDebugString(resp); : ASSERT_FALSE(resp.has_error()); Is it worth factoring this out into a helper method? http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/integration-tests/timestamp_advancement-itest.cc@354 PS2, Line 354: // Set a low Raft heartbeat. : FLAGS_leader_failure_max_missed_heartbeat_periods = 10; : FLAGS_raft_enable_pre_election = false; : FLAGS_enable_maintenance_manager = false; : : // Lower the heartbeat interval so the follower watermarks (that dictate when : // we can GC logs) get updated quickly. : FLAGS_raft_heartbeat_interval_ms = 10; : : scoped_refptr<TabletReplica> replica; : NO_FATALS(SetupClusterWithWritesInWAL(0, &replica)); : MiniTabletServer* ts = tserver(0); : : const MonoDelta kTimeout = MonoDelta::FromSeconds(30); : const string tablet_id = replica->tablet_id(); : : // Update one of the followers repeatedly to generate a bunch of config : // changes in all the replicas' WALs. : TServerDetails* leader; : ASSERT_OK(FindTabletLeader(ts_map_, tablet_id, kTimeout, &leader)); : vector<TServerDetails*> followers; : ASSERT_OK(FindTabletFollowers(ts_map_, tablet_id, kTimeout, &followers)); : ASSERT_FALSE(followers.empty()); : for (int i = 0; i < 100; i++) { : RaftPeerPB::MemberType type = i % 2 == 0 ? RaftPeerPB::NON_VOTER : RaftPeerPB::VOTER; : WARN_NOT_OK(ChangeReplicaType(leader, tablet_id, followers[0], type, kTimeout), : "Couldn't send a change config!"); : } : LOG(INFO) << "Finished inserting to the WALs"; : : ASSERT_EVENTUALLY([&] { : int64_t gcable_size; : ASSERT_OK(replica->GetGCableDataSize(&gcable_size)); : ASSERT_GT(gcable_size, 0); : LOG(INFO) << "GCing logs..."; : ASSERT_OK(replica->RunLogGC()); : }); : nit: most of this should be factored out into a helper method, since it's copy/pasted from above http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tablet/mvcc.h File src/kudu/tablet/mvcc.h: http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tablet/mvcc.h@190 PS2, Line 190: server serve http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tablet/mvcc.h@191 PS2, Line 191: CheckHasAdvancedTimestamps how about: CheckIsSafeTimeInitialized() ? http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tablet/mvcc.cc File src/kudu/tablet/mvcc.cc: http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tablet/mvcc.cc@52 PS2, Line 52: clean time I'm not sure we want to expose the concept of clean time to our users. Maybe safe time? Maybe "Safe time has not yet been initialized" ? http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tablet/mvcc.cc@52 PS2, Line 52: Aborted How about Uninitialized or maybe even ServiceUnavailable? http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tserver/tablet_service.cc@1767 PS2, Line 1767: *error_code = TabletServerErrorPB::TABLET_FAILED; Can we do something like service unavailable instead? According to the proto file, this means hard failure: // The tablet needs to be evicted and reassigned. TABLET_FAILED = 20; -- To view, visit http://gerrit.cloudera.org:8080/11428 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc0f77673e1f04a34ab1f5c1930bbaa2498b39bf Gerrit-Change-Number: 11428 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Sat, 15 Sep 2018 00:59:57 +0000 Gerrit-HasComments: Yes
