Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20901 )
Change subject: [log_block_manager] Write lock for deletion ...................................................................... Patch Set 16: (1 comment) > Build Failed > > http://jenkins.kudu.apache.org/job/pre_commit/193/ : FAILURE It's interesting that in all the build configurations some tests fail: https://jenkins.kudu.apache.org/job/pre_commit/193/pipeline-graph/ That looks a bit concerning. Did you try to figure out whether those failures are related to this update? http://gerrit.cloudera.org:8080/#/c/20901/4/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/20901/4/src/kudu/fs/log_block_manager.cc@1536 PS4, Line 1536: std::unique_lock<RWMutex> l(metadata_compact_lock_); What is the intention behind placing the guard here? In addition to changing from shared to exclusive lock, I'm not sure there is much to protect below in addition to what the original shared guard was protecting. And if so, wouldn't the same effect be achieved if just replacing the shared lock with exclusive lock at line 1548? BlockDeleted() just updates atomic fields, using constant fields as an input, and 'deleted_block_ids' is just an output parameter that's specific to each calling thread (even if it was shared, the better way of handling this would be using some local vector<BlockId>, and then moving/assigning the whole result vector later on, not doing it one-by-one inside of a loop). The 'lb' element's block_id() is also an immutable entity. -- To view, visit http://gerrit.cloudera.org:8080/20901 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I73712a5fd8eccf23621f8abda3e99ebdf10a769f Gerrit-Change-Number: 20901 Gerrit-PatchSet: 16 Gerrit-Owner: Ádám Bakai <aba...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 12 Jul 2024 00:54:52 +0000 Gerrit-HasComments: Yes
