Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/17871/3//COMMIT_MSG Commit Message: PS3: Have you tried deploying this on a cluster? Curious if you were able to see it solve the high disk usage issue you were seeing earlier. http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@208 PS3, Line 208: enable nit: "when enabling" http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@576 PS3, Line 576: // Try lock before reading metadata offset, consider it not full if lock failed. : if (!metadata_lock_.try_lock()) { : return false; : } Does this mean that if a metadata file is getting written to a lot, we may repeatedly consider it not full, even though it is? +1 to the idea of adding metadata size, if that helps us avoid this. http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@621 PS3, Line 621: bool check_compact_condition(bool lock) const { nit: maybe call this ShouldCompact()? http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@628 PS3, Line 628: if (lock) { nit: typically when we need both a locked and an unlocked version, that takes the form of T DoSomethingUnlocked() { // maybe DCHECK that the lock is held // ... does something } T DoSomething() { Lock l(lock_); DoSomethingUnlocked(); } http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@864 PS3, Line 864: // However, we're hosed if we can't open the new metadata file. In the event of a disk failure, rather than crashing, perhaps we should set some flag in the container to indicate to appenders that the container is bad? http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@2632 PS3, Line 2632: LogBlockContainerRefPtr c = FindOrDie(all_containers_by_name_, lb->container()->ToString()); Rather than looking up the container reference, could we use scoped_refptr<T>& operator=(T* p) to generate a scoped refptr? e.g. scoped_refptr<LogBlockContainer> self(lb->container()); -- To view, visit http://gerrit.cloudera.org:8080/17871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6 Gerrit-Change-Number: 17871 Gerrit-PatchSet: 3 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 29 Sep 2021 07:54:21 +0000 Gerrit-HasComments: Yes
