Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: introduce closed mvcc and stopped tablets ...................................................................... Patch Set 23: (22 comments) http://gerrit.cloudera.org:8080/#/c/7439/23//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/7439/23//COMMIT_MSG@49 PS23, Line 49: a new test stop_tablet-itest is added to ensure stopped leaders don't : complete writes and stopped followers do > having trouble parsing this. why do followers complete writes? Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc File src/kudu/integration-tests/stop_tablet-itest.cc: http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@57 PS23, Line 57: using std::thread; > warning: using decl 'thread' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@82 PS23, Line 82: *ret = *ret % > nit: %= Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@90 PS23, Line 90: T $0 P $1 > nit: usually we do P ... T ... (peer before tablet id) Hrm, looked around and it seems we mostly use "T xxx P xxx"? That's how it is in master/sys_catalog.h, and upon `grep`ing for " P " http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@102 PS23, Line 102: NO_PENDING_FATALS(); > I don't think this has any effect - it would just return if there is a pend Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@129 PS23, Line 129: // or -1 if it can't be found. > sort of odd to return -1 instead of bad status here. Up above GetTserverNum Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@181 PS23, Line 181: If this returns an error, the Tablet must : // have been stopped. > Similar to some comments on earlier revs, I find this doc hard to interpret I came to the realization that this might not be an appropriate place for this sort of comment. Errors can be passed up and down the stack and it shouldn't be the burden of one of the intermediate methods to bear the checking. http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@190 PS23, Line 190: // have been stopped. > same as above Same http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@230 PS23, Line 230: Status RewindSchemaForBootstrap(const Schema& schema, > warning: function 'kudu::tablet::Tablet::RewindSchemaForBootstrap' has a de Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@382 PS23, Line 382: Status DoMajorDeltaCompaction(const std::vector<ColumnId>& column_ids, > warning: function 'kudu::tablet::Tablet::DoMajorDeltaCompaction' has a defi Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@399 PS23, Line 399: void RegisterMaintenanceOps(MaintenanceManager* maintenance_manager); > warning: function 'kudu::tablet::Tablet::RegisterMaintenanceOps' has a defi Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@408 PS23, Line 408: // This method is not thread safe. > nit: it seems this is just copying from above, but would be nice if this cl Hrm, did this by pushing it under a lock since it's pretty cheap. The lock-holding semantics of the lock are a bit difficult to express. http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@465 PS23, Line 465: void set_state(State s) { > maybe name set_state_unlocked Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@466 PS23, Line 466: state_ = s; > can you add a DCHECK assert on state_lock_ here? Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@594 PS23, Line 594: // ones specified by 'expected_states'. > it is surprising to me that passing an empty set here, then, has the effect Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@597 PS23, Line 597: Status CheckActiveState(const std::set<State>& expected_states = {}) const; > given that we expect the set to be very small, I think a vector is probably Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@683 PS23, Line 683: potecting > typo Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc@299 PS23, Line 299: && > you mean ||? Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc@879 PS23, Line 879: expected_states.empty() > is this necessary? I was trying to replace: CHECK(state_ == kOpen || state_ == kBootstrapping); with { std::lock_guard<simple_spinlock> l(state_lock_); if (state_ == kStopped || state_ == kShutdown) { return Status::Aborted(); } CHECK(state_ == kOpen || state_ == kBootstrapping); } http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc@1028 PS23, Line 1028: shared_ptr<RowSet> input_rs) { > warning: the parameter 'input_rs' is copied for each invocation but only us Done http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc@1120 PS23, Line 1120: kOpen, kBootstrapping > how is this different than checking for {}? see other comment about maybe s Ack http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tserver/tablet_service.cc@1761 PS23, Line 1761: LOG(WARNING) << "Error setting up scanner with request " << SecureShortDebugString(*req); > this should probably include s.ToString() Done -- To view, visit http://gerrit.cloudera.org:8080/7439 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42 Gerrit-Change-Number: 7439 Gerrit-PatchSet: 23 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 15 Nov 2017 21:37:20 +0000 Gerrit-HasComments: Yes
