helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12308 )
Change subject: KUDU2665: LBM deletes the valid live block ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/12308/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12308/1/src/kudu/fs/log_block_manager.cc@531 PS1, Line 531: // Increases the referenced number. : // Positive means increase, negative means decrease. : int32_t IncrWritingRef(int32_t value) { return writing_ref_.IncrementBy(value); } > Nit: rename to "blocks_being_written_incr()" and change comment to "// Adju Done http://gerrit.cloudera.org:8080/#/c/12308/1/src/kudu/fs/log_block_manager.cc@535 PS1, Line 535: // Returns the referenced number by LogWritableBlock. : int32_t WritingRefCount() const { return writing_ref_.Load(); } > Nit: move this up to the "simple accessors" section and rename to blocks_be Done http://gerrit.cloudera.org:8080/#/c/12308/1/src/kudu/fs/log_block_manager.cc@640 PS1, Line 640: // The number of references by LogWritableBlock. : AtomicInt<int32_t> writing_ref_; > Nit: rename to blocks_being_written_ and change comment to "The number of L Done http://gerrit.cloudera.org:8080/#/c/12308/1/src/kudu/fs/log_block_manager.cc@1305 PS1, Line 1305: // Although this code looks like a TOCTOU violation, it is safe because of : // some additional LBM invariants: : // 1. When a container becomes full, it stays full for the process' lifetime. : // 2. A full container will never accrue another live block. Meaning, losing : // its last live block is a terminal state for a full container. > This needs to be updated to reflect the role of WritingRefCount(). Maybe th Done http://gerrit.cloudera.org:8080/#/c/12308/1/src/kudu/fs/log_block_manager.cc@1421 PS1, Line 1421: // However, due to KUDU-2665, it's not safe to visit container's next block : // offset and live block count at the same time. Thus, we need another flag : // to identify whether the container is being written instead of a lock. > Nit: this is more or less a retread of the "invariant" comment of TrySetDea Done http://gerrit.cloudera.org:8080/#/c/12308/1/src/kudu/fs/log_block_manager.cc@1424 PS1, Line 1424: if (container->full() && container->live_blocks() == 0 && container->WritingRefCount() == 0) { > This is getting unwieldy to have two copies of; please decompose into a sin Done -- To view, visit http://gerrit.cloudera.org:8080/12308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I894f32b1c164ae7770c92171850edd167dfaf8ad Gerrit-Change-Number: 12308 Gerrit-PatchSet: 1 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Thu, 31 Jan 2019 06:37:40 +0000 Gerrit-HasComments: Yes
