Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13412 )
Change subject: KUDU-2826: Add tail for mutation list to accelerate update in memrowset ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/13412/3/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/13412/3/src/kudu/tablet/memrowset.h@150 PS3, Line 150: // NOTE: this call is O(n) in the number of mutations, since it has to walk : // the linked list all the way to the end, checking if each mutation is a : // DELETE or REINSERT. We expect the list is usually short (low-update use : // cases) but if this becomes a bottleneck, we could cache the 'ghost' status : // as a bit inside the row header. Could you update this or remove it, now that the behavior has changed? Also, just want to make sure I understand this. It looks like the O(n) behavior was known behavior that was a conservative check for correctness, and now that the code is battle tested, we're comfortable making the optimization in this patch. Is that correct? http://gerrit.cloudera.org:8080/#/c/13412/3/src/kudu/tablet/memrowset.cc File src/kudu/tablet/memrowset.cc: http://gerrit.cloudera.org:8080/#/c/13412/3/src/kudu/tablet/memrowset.cc@77 PS3, Line 77: "Failed to decode: " << mut_tail->changelist().ToString(*schema()) : << " (" << s.ToString() << ")"; nit: this may be slightly more readable if written with Substitute(). http://gerrit.cloudera.org:8080/#/c/13412/3/src/kudu/tablet/memrowset.cc@80 PS3, Line 80: if (decoder.is_delete()) { : is_ghost = true; : } : return is_ghost; nit: could simplify this to be `return decoder.is_delete()`. And L72 to be `return false`, in which case we wouldn't need the `is_ghost` variable. -- To view, visit http://gerrit.cloudera.org:8080/13412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I945f332f8241ecb3cc6ab66e67ee3ee2b4e49be8 Gerrit-Change-Number: 13412 Gerrit-PatchSet: 3 Gerrit-Owner: ZhangYao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Yao Xu <[email protected]> Gerrit-Reviewer: ZhangYao <[email protected]> Gerrit-Comment-Date: Tue, 28 May 2019 04:00:29 +0000 Gerrit-HasComments: Yes
