David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/9641 )
Change subject: Add a way to pin clean time advancement ...................................................................... Patch Set 7: Verified+1 (24 comments) Oops had some unposted old comments. The failure is due to an unrelated flake. 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 Done http://gerrit.cloudera.org:8080/#/c/9641/3//COMMIT_MSG@15 PS3, Line 15: add > nit: adds Done 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? Done http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc-test.cc@248 PS3, Line 248: the > nit: drop Done 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. Done http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc-test.cc@268 PS3, Line 268: advance > advances Done http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc-test.cc@270 PS3, Line 270: safe_time = Timestamp(55); : mgr.AdjustSafeTime(Timestamp(55)); : ASSERT_EQ(safe_time, mgr.GetCleanTimestamp()); > I'm not sure this corresponds to the comment above: the transaction timesta good catch! thanks. fixed 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@288 PS3, Line 288: struct ScopedCleanTimeAdvancePin { > this should probably be marked DISALLOW_COPY_AND_ASSIGN since an accidental Done 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 Done http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@290 PS3, Line 290: > nit: spacing/alignment Done http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@295 PS3, Line 295: mvcc->UnPinCleanTimeAdvance(); > is this idempotent? It surprises me that we don't need to pass in pinned_ti I avoided making sure that this unpins only once because it makes usage slightly better in the case of bail out. happy to enforce the check if you feel strongly. http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@303 PS3, Line 303: Timestamp pinned_timestamp; > nit: const? Done http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@370 PS3, Line 370: PinCleanTimeAdvanceTo > nit: how about PinCleanTimeAdvancement() and UnpinCleanTimeAdvancement(). Done http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@370 PS3, Line 370: void PinCleanTimeAdvanceTo(Timestamp timestamp); > is there a limitation that there can be only a single pin at a time? What h yes it crashed added a comment to make that clear http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@375 PS3, Line 375: // If clean time is not pinned this has no effect. > should we be able to FATAL in that case? Seems a little odd that we could h I didn't make this crash in this case just because the raii use is slightly cleaner. happy to change it if you feel strongly 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 it does! thanks 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. > yea, I commented on the header before looking at the impl, and that wasn't Done 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 yes, but I din't want to talk about prepare stuff here. added a comment to the header to make it clean that this can only be called once. 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. > Yep, maybe it's worth extend the reasoning a bit in the comment. Done 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 Done http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc@218 PS3, Line 218: CHECK_EQ(clean_time_pin_, Timestamp::kInvalidTimestamp); > Are there any other requirements on the clean-time pin? eg it should be ill good point. added that check and a corresponding comment. http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc@229 PS3, Line 229: MvccManager::AdjustCleanTime() { > While you're in the area, would you mind renaming this to AdjustCleanTimeUn Done 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::kInvalidTimesta yeah, kinvalid timestamp is max - 1, do taking the min is dafe 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_}); > Apparently, it does :) Done -- 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: 7 Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 25 Jul 2018 00:37:34 +0000 Gerrit-HasComments: Yes
