You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Ashwani Raina (Code Review)" <ge...@cloudera.org> on 2026/04/29 20:04:44 UTC
[kudu-CR] KUDU-829 Add online gc op to clean orphaned blocks
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/24125 )
Change subject: KUDU-829 Add online gc op to clean orphaned blocks
......................................................................
Patch Set 2:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/24125/2//COMMIT_MSG
Commit Message:
PS2:
> I'd expect this patch to be posted on top of https://gerrit.cloudera.org/#/
After 24092 is merged, I am planning to resolve all those conflicts and rev this PS.
http://gerrit.cloudera.org:8080/#/c/24125/2//COMMIT_MSG@9
PS2, Line 9: manintenance
> maintenance
Done
http://gerrit.cloudera.org:8080/#/c/24125/2//COMMIT_MSG@34
PS2, Line 34: persisten
> persistent
Done
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet.cc@393
PS2, Line 393: if (metrics_) {
> If this condition necessary?
Just in case ctor is called with null 'metric_registry'
Moved SetMetrics call inside 'if (metric_registry)' condition and removed the null check to avoid extra check.
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_metadata.cc@306
PS2, Line 306: LOG_WITH_PREFIX(WARNING) << Substitute(
: "$0 orphaned block(s) could not be deleted from disk during tablet deletion "
: "and will occupy untracked disk space. "
: "Run 'kudu fs check --repair' to reclaim this disk space.",
: orphaned_blocks_.size());
> Move logging outside of the critical section to avoid needlessly holding lo
Good catch!
Moved log outside lock and switched to swap statement inside lock.
There are a bunch of other places in the code where 'logging under lock' anti-pattern is seen:
++
src/kudu/client/meta_cache.cc-300- VLOG(2) << "Tablet " << tablet_id_ << ": Current remote replicas in meta cache: "
src/kudu/clock/mock_ntp.cc-31- VLOG(1) << "Current clock time: " << mock_clock_time_usec_
src/kudu/clock/mock_ntp.cc-40- VLOG(1) << "Updating to time: " << now_usec;
src/kudu/clock/mock_ntp.cc-47- VLOG(1) << "Updating to error: " << max_error_usec;
src/kudu/consensus/raft_consensus.cc-606- LOG_WITH_PREFIX_UNLOCKED(INFO) << "Received request to transfer leadership"
src/kudu/tools/tool_action_local_replica.cc-348- LOG(WARNING) << "Start to copy tablet " << tablet_id;
src/kudu/util/mt-threadlocal-test.cc-113- LOG(INFO) << tid_ << ": deleting self from registry...";
src/kudu/util/maintenance_manager-test.cc-138- DLOG(INFO) << "Performing op " << name();
++
I will send a separate patch to address those.
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_metadata.cc@624
PS2, Line 624: }
> should we also have:
Done
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_metadata.cc@657
PS2, Line 657: if (to_retry.empty()) return;
> style nit for here and elsewhere: use scope braces even for a one-liner 'if
Done
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_metrics.h
File src/kudu/tablet/tablet_metrics.h:
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_metrics.h@87
PS2, Line 87: Blocks cleaned and time spent
> I'm not sure I can fathom a dual metric like described.
Done
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_metrics.cc
File src/kudu/tablet/tablet_metrics.cc:
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_metrics.cc@254
PS2, Line 254: "Number of orphaned blocks deleted during tablet metadata load at startup."
> nit: add extra trailing space
Done
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops-test.cc
File src/kudu/tablet/tablet_mm_ops-test.cc:
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops-test.cc@246
PS2, Line 246: againa
> nit: again
Done
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops.h
File src/kudu/tablet/tablet_mm_ops.h:
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops.h@169
PS2, Line 169: are created
> appear
Done
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops.h@169
PS2, Line 169: compactioni
> compaction
Done
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops.h@202
PS2, Line 202: = 0
> style nit: move initialization into one place -- if initializing some of th
Done
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops.cc
File src/kudu/tablet/tablet_mm_ops.cc:
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops.cc@392
PS2, Line 392: size_t num_pending = tablet_->metadata()->num_orphaned_blocks();
: if (num_pending == 0) {
> nit: get rid of the temporary 'num_pending' variable
num_pending is used at line 419
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops.cc@405
PS2, Line 405: const int
> Make this constexpr
Done
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops.cc@419
PS2, Line 419: stats->set_data_retained_bytes(static_cast<int64_t>(num_pending));
> set_data_retained_bytes is being set to the raw block count rather than an
This was intentionally put to avoid block-size lookups and deliberately put it on lower priority (with probability of ending up with orphaned blocks being quite low). As the number of block increases, the chances of this value exceeding 'retained bytes' for other ops also increases thereby increasing the probability of this being scheduled by mm.
However, it is possible to run into a perpetual scheduling denial because of a small number of orphaned block number. Also, this doesn't go well with the semantics.
I think some sort of scaling or perf_improvement factor can be introduced to improve its chances to run, maybe in a separate patch.
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops.cc@424
PS2, Line 424: LOG(INFO) << LogPrefix() << "Retrying deletion of pending orphaned blocks";
> What's the purpose of having this log line? If this is for debugging, cons
Done
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops.cc@448
PS2, Line 448: std::string OrphanedBlockGCOp::LogPrefix() const {
: return tablet_->LogPrefix();
: }
> Why to have this if TabletOpBase::LogPrefix() does exactly same already?
Removed here as well as from other ops.
--
To view, visit http://gerrit.cloudera.org:8080/24125
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I732133b460df2fe4c91d05f420dda6f0274d440e
Gerrit-Change-Number: 24125
Gerrit-PatchSet: 2
Gerrit-Owner: Ashwani Raina <ar...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Wed, 29 Apr 2026 20:04:44 +0000
Gerrit-HasComments: Yes