Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets ...................................................................... Patch Set 25: (10 comments) http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc File src/kudu/integration-tests/stop_tablet-itest.cc: http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc@132 PS25, Line 132: leader = i; > why not just do: *leader_num = i; return Status::OK() here? Done http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc@137 PS25, Line 137: return Status::NotFound("Could not find the tablet leader"); > and then just unconditionally return NotFound here if you hit the end of th Done http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc@149 PS25, Line 149: NO_PENDING_FATALS(); > NO_FATALS(...) on the prior line already ensures that there are no pending Done http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc@180 PS25, Line 180: NO_PENDING_FATALS(); > same Done http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.h@400 PS25, Line 400: // This method is are thread-safe. > typo Done http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.h@590 PS25, Line 590: vector > passing a vector here with a default-constructed argument like this seems t Hrm, seems like constexpr initializer_lists don't work. Needs to be static const. And this seems to be getting more complex than it needs to be, so I'm going to fall back on a macro. http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.h@677 PS25, Line 677: 'maintenance_ops_'. > this is somewhat of a code smell IMO -- locks should protect data, not code Done http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.cc@289 PS25, Line 289: set_state_unlocked(kBootstrapping); > is it possible to race against a Tablet::Close() call that's concurrent wit Done http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.cc@313 PS25, Line 313: CHECK_EQ(state_, kBootstrapping); > what happens if the tablet gets Stop()ed while it was in the progress of bo I've made this return IllegalStatus() and plumbed that up to bootstrap. Failures in bootstrap leave the tablet failed. http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.cc@1365 PS25, Line 1365: maintenance_ops_.push_back(rs_compact_op.release()); > doesn't this need to hold the lock, if someone could call a concurrent Stop 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: 25 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 16 Nov 2017 23:47:11 +0000 Gerrit-HasComments: Yes
