Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9641 )
Change subject: Add a way to pin clean time advancement ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/9641/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9641/3//COMMIT_MSG@12 PS3, Line 12: mvcc nit: MVCC http://gerrit.cloudera.org:8080/#/c/9641/3//COMMIT_MSG@20 PS3, Line 20: raii nit: RAII http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc-test.cc File src/kudu/tablet/mvcc-test.cc: http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc-test.cc@226 PS3, Line 226: transaction nit: transactions? http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc-test.cc@255 PS3, Line 255: // Pin clean time advancement to an instant before the in-flight nit: add a stop in the end of the sentence. http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h File src/kudu/tablet/mvcc.h: http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@289 PS3, Line 289: _ nit: extra suffix. I think it should be possible to have mvcc(mvcc) in the list of initializers. http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@290 PS3, Line 290: nit: spacing/alignment http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@303 PS3, Line 303: Timestamp pinned_timestamp; nit: const? http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc File src/kudu/tablet/mvcc.cc: http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc@216 PS3, Line 216: std::lock_guard<LockType> l(lock_); nit: does it make sense to add DCHECK_NE(Timestamp::kInvalidTimestamp, timestamp); ? http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc@217 PS3, Line 217: // There should only be one pin at a time. > Hrmm.. Is this because the prepare thread is single-threaded, and so we are Yep, maybe it's worth extend the reasoning a bit in the comment. http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc@218 PS3, Line 218: clean_time_pin_, Timestamp::kInvalidTimestamp nit: maybe, swap the arguments for better readability? I mean, usually in ASSERT_EQ() and others the expected values comes first. http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc@233 PS3, Line 233: std::min({earliest_in_flight_, safe_time_, clean_time_pin_}); Does it work as expected when clean_time_pin_ == Timestamp::kInvalidTimestamp ? -- To view, visit http://gerrit.cloudera.org:8080/9641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If863d28016aee98672d94ec5dc8a8630e1cbf5c8 Gerrit-Change-Number: 9641 Gerrit-PatchSet: 3 Gerrit-Owner: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 21 Mar 2018 18:36:25 +0000 Gerrit-HasComments: Yes
