Andrew Wong 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) 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? Done 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 c Done 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 Done http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tablet/mvcc.h@191 PS2, Line 191: CheckHasAdvancedTimestamps > how about: CheckIsSafeTimeInitialized() ? Done 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: Aborted > How about Uninitialized or maybe even ServiceUnavailable? Done 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. Mayb Done 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 prot Hrm, yeah I agree TABLET_FAILED is inappropriate. The closest thing we have to unavailable is the rpc status ERROR_UNAVAILABLE, but that doesn't seem to fit perfectly either, and I'm hesitant to add a new tserver error code. How about TABLET_NOT_RUNNING? That's what we would use if the tablet were bootstrapping, and it isn't too far of a stretch to think of this state of waiting for consensus as an extension of bootstrapping. -- 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 <aw...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 18 Sep 2018 18:47:23 +0000 Gerrit-HasComments: Yes
