Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs ...................................................................... Patch Set 21: (10 comments) http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h File src/kudu/tablet/mvcc.h: http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@277 PS21, Line 277: SetClosed nit: Close() ? http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@280 PS21, Line 280: is_closed nit: Perhaps is_open() would be more intuitive than is_closed()? This is because closed is a negative and checking for is not closed is a bit of a boolean trap: https://ariya.io/2011/08/hall-of-api-shame-boolean-trap http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@280 PS21, Line 280: inline Is "inline" useful here? I thought methods defined in their header declarations were generally inlined anyway. http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@281 PS21, Line 281: .load() Can we get away with std::memory_order_relaxed? http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@316 PS21, Line 316: CheckNotClosed nit: CheckOpen() seems more intuitive to me per the above http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@382 PS21, Line 382: closed_ perhaps open_ and default to true? http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.cc File src/kudu/tablet/mvcc.cc: http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.cc@98 PS21, Line 98: closed_.load() nit: is_closed() http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.cc@280 PS21, Line 280: did did not http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@121 PS21, Line 121: Unlike Shutdown(), this is not a lifecycle change, but rather a functional : // one. Calls to this are orthogonal to calls to Shutdown(). It's not clear when one vs. the other should be called. Can you clarify here? Also, is it ever possible to go from Stopped() back to running? If not, this seems like a lifecycle change to me, despite the header comment here. http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet_replica.cc@293 PS21, Line 293: tablet_->Shutdown(); We call Tablet::Shutdown() here. So why not just have Tablet::Shutdown() call Tablet::Stop()? -- 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: 21 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, 31 Oct 2017 23:00:20 +0000 Gerrit-HasComments: Yes
