Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8605 )
Change subject: tablet: mark delta tracker read-only on error ...................................................................... Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/8605/5/src/kudu/tablet/delta_tracker.h File src/kudu/tablet/delta_tracker.h: http://gerrit.cloudera.org:8080/#/c/8605/5/src/kudu/tablet/delta_tracker.h@362 PS5, Line 362: // ensure type-correctness of the delta store lists. I think we'll forget what this means. Maybe we should just say that errors may leave the internal state such that future maintenance ops would not be safe to run, and leave out the specifics. http://gerrit.cloudera.org:8080/#/c/8605/5/src/kudu/tablet/delta_tracker.h@363 PS5, Line 363: Status read_only_status_; do we need a full status here? This will take 8 bytes up in the object size. Is a bool sufficient? If so, if we order it next to one of the existing bools, it should pack into existing free padding without increasing the overall struct size. http://gerrit.cloudera.org:8080/#/c/8605/5/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: http://gerrit.cloudera.org:8080/#/c/8605/5/src/kudu/tablet/delta_tracker.cc@726 PS5, Line 726: if (PREDICT_FALSE(!s.ok())) { do we still want to FATAL in the case of a non-disk-error here? otherwise it's possible that we'd have some bug that would just mark a deltatracker read-only, but not actually mark the tablet failed, and then we'd be "stuck" with this DMS occupying memory but being unflushable -- To view, visit http://gerrit.cloudera.org:8080/8605 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib950048e9cd0929a10714ab1cc2bd829835afced Gerrit-Change-Number: 8605 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 21 Nov 2017 06:01:30 +0000 Gerrit-HasComments: Yes
