Todd Lipcon 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: (6 comments) 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 copy would result in an extra "unpin" 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_timestamp here. Even if we are currently allowing only a single "pin" at a time, perhaps it has value for CHECK purposes 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 happens if there is already a pin set up? Crash? 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 have mis-matched calls in any case 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@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. yea, I commented on the header before looking at the impl, and that wasn't entirely clear. 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 illegal to pin the cleantime to an earlier time than the current cleantime? -- 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 <[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: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 21 Mar 2018 19:51:21 +0000 Gerrit-HasComments: Yes
