Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs ...................................................................... Patch Set 23: (17 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? 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@82 PS23, Line 82: *ret = *ret % nit: %= 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) Probably better to be consistent for easier grepping 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 pending fatal. it needs to be after any _calls_ to this function 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 GetTserverNumToFail would end up returning -1 in the case this returns -1 which seems not right (may lead to crash?) 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. Is this a guarantee that: if this returns an error, then the Tablet state will have already transitioned to stopped state? In that case I think it is better to say "If this returns an error, it is guaranteed that the tablet will be in stopped state upon return." Or, if, say, we have some storage problem in our compaction code or something, this could return an error but _not_ have the tablet in stopped state, in which case the caller should probably be CHECKing? http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@190 PS23, Line 190: // have been stopped. same as above 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 clarified the thread safety a bit better. Should it be called while holding some other lock? When's it safe to call? 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 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? 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 that it does. Maybe this should be two separate methods? 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 more efficient here than std::set http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@683 PS23, Line 683: potecting typo 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 ||? http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc@879 PS23, Line 879: expected_states.empty() is this necessary? 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 splitting this into separate methods that are a little easier to understand 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() -- 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: Tue, 14 Nov 2017 00:08:52 +0000 Gerrit-HasComments: Yes
