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/03/21 14:16:31 UTC
[kudu-CR] [WIP] Add online gc op to clean orphaned blocks
Ashwani Raina has uploaded this change for review. ( http://gerrit.cloudera.org:8080/24125
Change subject: [WIP] Add online gc op to clean orphaned blocks
......................................................................
[WIP] Add online gc op to clean orphaned blocks
This patch adds a background manintenance GC operation that makes use
of persisted information about set of orphaned blocks that could not be
deleted earlier due to transient errors, and tries to delete those in
the background.
Main highlights:
- Add RetryOrphanedBlockDeletion as the main driving function that does
fetching of orphaned blocks, call DeleteOrphanedBlocks for the set of
orphaned blocks.
- Ensure DeleteOrphanedBlocks doesn't erase all the block entries from
the set of orphaned blocks when either partial or nothing from the set
was successfully deleted. The blocks with failed deletions are to be
used in next iteration by either GC or LoadSuperBlock.
- With this change, ensure when superblock load kicks in, we try to
delete the stale orphaned blocks only once. Irrespective of what the
outcome is from deletion, erase all the block entries from in-memory
set. This avoids possibility of those blocks being retried for delete
unnecessarily.
- Add unit tests to:
* Test that OrphanedGCOp runs when there orphaned blocks to be deleted.
* Measure performance benchmark for the KUDU-1060 startup overhead.
* Test startup overhead with many tombstoned tablets.
TODO:
- Explore viability of background GC logic for persistent I/O errors.
If possible find ways to avoid indefinite retries in such cases.
- Add more details in the commit message.
- Run more benchmarking tests.
Change-Id: I732133b460df2fe4c91d05f420dda6f0274d440e
---
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_metrics.cc
M src/kudu/tablet/tablet_metrics.h
M src/kudu/tablet/tablet_mm_ops.cc
M src/kudu/tablet/tablet_mm_ops.h
9 files changed, 881 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/24125/1
--
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: newchange
Gerrit-Change-Id: I732133b460df2fe4c91d05f420dda6f0274d440e
Gerrit-Change-Number: 24125
Gerrit-PatchSet: 1
Gerrit-Owner: Ashwani Raina <ar...@gmail.com>
[kudu-CR] KUDU-829 Add online gc op to clean orphaned blocks
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins 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 3: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/2529/ : FAILURE
--
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: 3
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 21:14:33 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-829 Add online gc op to clean orphaned blocks
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins 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 3:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/2529/
--
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: 3
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:05:11 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-829 Add online gc op to clean orphaned blocks
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/24125
to look at the new patch set (#5).
Change subject: KUDU-829 Add online gc op to clean orphaned blocks
......................................................................
KUDU-829 Add online gc op to clean orphaned blocks
This patch adds a background maintenance GC operation that makes use
of persisted information about set of orphaned blocks that could not be
deleted earlier due to transient errors, and tries to delete those in
the background. Following retain-on-failure semantics, failure paths
ensure the blocks (for which clean-up failed) are retained for a
subsequent retry from either GC, Flush, or superblock-load.
Main highlights:
- Add RetryOrphanedBlockDeletion as the main driving function that does
fetching of orphaned blocks, call DeleteOrphanedBlocks for the set of
orphaned blocks.
- Ensure DeleteOrphanedBlocks doesn't erase all the block entries from
the set of orphaned blocks when either partial or nothing from the set
was successfully deleted. The blocks with failed deletions are to be
used in next iteration by either GC or LoadFromSuperBlock or Flush.
- With this change, ensure when superblock load kicks in, we try to
delete the stale orphaned blocks only once. Irrespective of what the
outcome is from deletion, erase all the block entries from in-memory
set. This avoids possibility of those blocks being retried for delete
unnecessarily.
- LBM Restore Logic: Restore failed blocks to in-memory LBM state if
lower layer error was hit (maybe inside RemoveBlockIdsFromMetadata).
- Enhanced Metrics:
* Add `orphaned_blocks_count` gauge and cleanup path counters.
* Track cleanup by GC, flush, and load operations separately.
* Add failure counter for deletion attempts.
- Add unit tests to:
* Test that OrphanedGCOp runs when orphaned blocks are to be deleted.
* Test for retain-on-failure, and backoff logic.
* Cover both implementations: NativeMeta and RdbMeta.
* Add integration tests for all cleanup paths (GC, flush, load).
* Measure performance benchmark for the KUDU-1060 startup overhead.
* Test startup overhead with many tombstoned tablets.
- To address the issue of frequent scheduling of GC operation if there
are persistent I/O errors, add backoff logic that increments the time
duration exponentially after each repeated failure hit during cleaning
of orphaned blocks (possibly due to some persistent disk error). Until
this time duration passes, no orphaned blocks GC op is scheduled.
=== KUDU-1060 Startup Overhead Benchmark ===
Tablets : 1000
Orphaned blocks / tablet : 1000000
With cleanup + Flush() : 613387 ms
Without cleanup + Flush() : 253331 ms
Extra overhead (Flush x N) : 360056 ms
Avg overhead per tablet : 360 ms
=== KUDU-1060 One-Time-Cost Benchmark ===
Tablets : 1000
Orphaned blocks / tablet : 1000000
Pass 1 - 1st restart (fix ON, dirty) : 629216 ms [cleanup+Flush for all tablets]
Pass 2 - 2nd restart (fix ON, clean) : 114 ms [superblocks already clean]
Pass 3 - clean baseline (fix OFF, clean) : 112 ms [no orphaned blocks to parse]
Pass 4 - legacy baseline (fix OFF, dirty) : 262499 ms [parses orphan IDs, no Flush]
Pass1 overhead vs Pass4 legacy : 366717 ms
Pass2 vs Pass3 clean baseline : 2 ms (should be ~0)
Follow-up work(to be addressed in separate patch/es):
- Investigate possibility of un-fsynced DELETE record lost on crash.
Explore the feasibility of syncing container metadata to ensure DELETE
records land in container before wirting clean superblock.
- Hitting WritableBlock::Abort() path after create record can also lead
to leaked space in blocks.
Change-Id: I732133b460df2fe4c91d05f420dda6f0274d440e
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_metrics.cc
M src/kudu/tablet/tablet_metrics.h
M src/kudu/tablet/tablet_mm_ops-test.cc
M src/kudu/tablet/tablet_mm_ops.cc
M src/kudu/tablet/tablet_mm_ops.h
12 files changed, 2,151 insertions(+), 108 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/24125/5
--
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: newpatchset
Gerrit-Change-Id: I732133b460df2fe4c91d05f420dda6f0274d440e
Gerrit-Change-Number: 24125
Gerrit-PatchSet: 5
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>
[kudu-CR] KUDU-829 Add online gc op to clean orphaned blocks
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin 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 5:
(19 comments)
Did second review pass, posted a few high-level comments and a bunch of nits.
I think I'll need to revisit this once more time at least -- I didn't have a chance to look through the updates in ts_recovery-itest.cc.
http://gerrit.cloudera.org:8080/#/c/24125/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/24125/5//COMMIT_MSG@41
PS5, Line 41: KUDU-1060 startup overhead
And now it has become confusing after I obtained more context on this patch.
From its description, KUDU-1060 is about not updating tablet's metadata after the orphaned blocks were cleaned up, so tablet metadata for deleted tablets always has a lot of orphaned blocks in it upon start up. And KUDU-1060 has been marked as FIXED with https://github.com/apache/kudu/commit/86440ef3dcc95d7c902bd908a4e61cea05370d4a
With that, do you mind replacing KUDU-1060 here and elsewhere in this patch (where applicable) with a few words to express what you actually mean in plain English?
Maybe, opening a new JIRA item with the specifics that you refer to is a better option. Using that new JIRA item instead KUDU-1060 would be easier to comprehend.
Also, did you consider separating the part of cleaning up the information on orphaned blocks of tombstoned tablets upon start-up into its own patch and tracking that under a separate JIRA ticket?
http://gerrit.cloudera.org:8080/#/c/24125/5//COMMIT_MSG@49
PS5, Line 49: KUDU-1060
This is confusing
http://gerrit.cloudera.org:8080/#/c/24125/5//COMMIT_MSG@70
PS5, Line 70: wirting
writing
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/fs/log_block_manager.cc@184
PS5, Line 184: hidden
Should this be 'unsafe' instead?
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/fs/log_block_manager.cc@205
PS5, Line 205: TAG_FLAG(log_block_manager_inject_latency_load_container_ms, hidden);
hidden' isn't needed if the flag is marked 'unsafe':
// - "unsafe":
// These flags are for internal use only (e.g. testing), and changing
// them away from the defaults may result in arbitrarily bad things
// happening. These flags are automatically excluded from user-facing
// documentation even if they are not also marked 'hidden'.
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet.cc@595
PS5, Line 595: metadata_->SetMetrics(nullptr);
Shouldn't we do this before un-publishing the metric_entity_ at line 579?
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_metadata-test.cc
File src/kudu/tablet/tablet_metadata-test.cc:
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_metadata-test.cc@1142
PS5, Line 1142: KUDU-1060
This is confusing. Please update this according to the comment at https://gerrit.cloudera.org/#/c/24125/5//COMMIT_MSG@41
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_metadata.h@491
PS5, Line 491: = nullptr
nit: move initialization to the intitializer list in the constructor?
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: std::lock_guard l(data_lock_);
: to_clear.swap(orphaned_blocks_);
: }
:
: // Do the logging outside the l
> Good catch!
Sure, thanks!
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_metadata.cc@72
PS5, Line 72: DEFINE_bool(enable_orphaned_block_cleanup_on_load, true,
Maybe, separate this and all the related code into its own changelist, not mixing it with
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_metadata.cc@78
PS5, Line 78: Disable to measure the baseline startup cost without "
: "this cleanup, e.g. to quantify the improvement this fix provides.
Remove this part -- it's not directly relevant to the description of the flag.
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_metadata.cc@81
PS5, Line 81: TAG_FLAG(enable_orphaned_block_cleanup_on_load, hidden);
I'm curious to know more about the justification to make this flag hidden.
Keep in mind that tablet bootstrapping performance is a very sensitive topic for large Kudu clusters. I think that people would like to have more control of various knobs that affect it. Adding 'hidden' tag remove this flag from the set of auto-documented flags, so this is now completely obscure.
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_metrics.h
File src/kudu/tablet/tablet_metrics.h:
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_metrics.h@85
PS5, Line 85: // Current number of orphaned blocks pending cleanup. Incremented each time a
Could you please consolidate the per-metric comments and its description provided with METRIC_DEFINE_xxx() in tablet_metrics.cc, and remove the comments from here?
I don't think it make sense to duplicate the description of those metrics here.
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_metrics.cc
File src/kudu/tablet/tablet_metrics.cc:
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_metrics.cc@283
PS5, Line 283: kInfo
Why not 'kDebug', similar to 'orphaned_block_gc_duration'?
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_mm_ops-test.cc
File src/kudu/tablet/tablet_mm_ops-test.cc:
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_mm_ops-test.cc@209
PS5, Line 209: google::FlagSaver flag_saver;
How long does it takes to run this scenario? If that's 3 seconds and more, add SKIP_IF_SLOW_NOT_ALLOWED()
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_mm_ops-test.cc@243
PS5, Line 243: →
nit for here and elsewhere: please update to use plain ASCII characters
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_mm_ops-test.cc@274
PS5, Line 274: →
nit: remove unicode, use plain ASCII
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@419
PS2, Line 419: // As long as last failure time hasn't crossed backoff duration, keep the
> This was intentionally put to avoid block-size lookups and deliberately put
Don't we have a rough estimate for a block size upfront?
I guess the main point is to run this OrphanedBlockGCOp only when there aren't any other types of operations scheduled. The whole point of the existing background operations is to avoid eventual performance degradation because of growing MRS sizes, accumulated on-disk deltas, etc. For orphaned blocks, there isn't a risk of degraded performance -- the only problem is wasted disk space and slower bootstrapping of tablets.
Probably, updating the comment to reflect on that could make it easier to comprehend.
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_mm_ops.cc
File src/kudu/tablet/tablet_mm_ops.cc:
http://gerrit.cloudera.org:8080/#/c/24125/5/src/kudu/tablet/tablet_mm_ops.cc@426
PS5, Line 426: // Each pending block represents at least one failed deletion attempt, so
: // treat the count as a rough proxy for retained data to drive scheduling
: // priority relative to other maintenance ops.
: stats->set_data_retained_bytes(static_cast<int64_t>(num_pending));
I guess this is only relevant for selecting among operations of OrphanedBlockGCOp type, right? Because the rest of the ops provide much higher values for data_retained_bytes since they estimates the actual amount of data in rowsets, etc.
--
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: 5
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: Fri, 08 May 2026 21:00:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-829 Add online gc op to clean orphaned blocks
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin 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:
(15 comments)
A quick fast pass.
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/#/c/24092/, but it doesn't seem to be the case. Otherwise, there will be conflicts with https://gerrit.cloudera.org/#/c/24092/ upon an attempt to push this patch into the repo after review.
http://gerrit.cloudera.org:8080/#/c/24125/2//COMMIT_MSG@9
PS2, Line 9: manintenance
maintenance
http://gerrit.cloudera.org:8080/#/c/24125/2//COMMIT_MSG@34
PS2, Line 34: persisten
persistent
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?
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 locks during IO.
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' as in the rest of this code below
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_metadata.cc@722
PS2, Line 722: they are retried on the next metadata flush
This complicates reading of the orphaned_block_cleanup_failures metric -- it becomes senseless. Perhaps, it's necessary to introduce an extra gauge to see the number of blocks failed deletion last time.
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.
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
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops.h@169
PS2, Line 169: compactioni
compaction
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 the variables in the constructor initializers' list (e.g., the 'running_' field), move this there
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
http://gerrit.cloudera.org:8080/#/c/24125/2/src/kudu/tablet/tablet_mm_ops.cc@405
PS2, Line 405: const int
Make this constexpr
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, consider switching to VLOG(1) or similar.
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?
--
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: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Tue, 24 Mar 2026 23:28:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-829 Add online gc op to clean orphaned blocks
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins 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: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/2412/ : FAILURE
--
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: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 23 Mar 2026 11:01:28 +0000
Gerrit-HasComments: No
[kudu-CR] [WIP] Add online gc op to clean orphaned blocks
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/24125 )
Change subject: [WIP] Add online gc op to clean orphaned blocks
......................................................................
Patch Set 1:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/2411/
--
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: 1
Gerrit-Owner: Ashwani Raina <ar...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 21 Mar 2026 14:16:40 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-829 Add online gc op to clean orphaned blocks
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Marton Greber 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:
(4 comments)
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@624
PS2, Line 624: }
should we also have:
metrics_->orphaned_block_cleanup_failures->IncrementBy(s.blocks_failed)
if non-zero?
Since blocks_failed is calculated above?
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
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
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@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 actual byte estimate. Since the maintenance manager uses this field to compare scheduling priority across all ops (e.g. against UndoDeltaBlockGCOp, which reports real byte sizes), a small number of orphaned blocks will always be outweighed. Should this use a minimum floor value, or at least a per-block byte estimate, to ensure the op is not perpetually starved when other ops report large byte counts?
--
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: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Wed, 25 Mar 2026 12:19:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-829 Add online gc op to clean orphaned blocks
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/24125
to look at the new patch set (#2).
Change subject: KUDU-829 Add online gc op to clean orphaned blocks
......................................................................
KUDU-829 Add online gc op to clean orphaned blocks
This patch adds a background manintenance GC operation that makes use
of persisted information about set of orphaned blocks that could not be
deleted earlier due to transient errors, and tries to delete those in
the background.
Main highlights:
- Add RetryOrphanedBlockDeletion as the main driving function that does
fetching of orphaned blocks, call DeleteOrphanedBlocks for the set of
orphaned blocks.
- Ensure DeleteOrphanedBlocks doesn't erase all the block entries from
the set of orphaned blocks when either partial or nothing from the set
was successfully deleted. The blocks with failed deletions are to be
used in next iteration by either GC or LoadSuperBlock.
- With this change, ensure when superblock load kicks in, we try to
delete the stale orphaned blocks only once. Irrespective of what the
outcome is from deletion, erase all the block entries from in-memory
set. This avoids possibility of those blocks being retried for delete
unnecessarily.
- Add unit tests to:
* Test that OrphanedGCOp runs when there orphaned blocks to be deleted.
* Measure performance benchmark for the KUDU-1060 startup overhead.
* Test startup overhead with many tombstoned tablets.
- To address the issue of frequent scheduling of GC operation if there
are persistent I/O errors, add backoff logic that increments the time
duration exponentially after each repeated failure hit during cleaning
of orphaned blocks (possibly due to some persisten disk error). Until
this time duration passes, no orphaned blocks GC op is scheduled.
- Add unit test to verify the backoff logic to slow down frequent
scheduling orphaned blocks GC op if consecutive cleaning failures are
seen.
=== KUDU-1060 Startup Overhead Benchmark ===
Tablets : 1000
Orphaned blocks / tablet : 1000000
With cleanup + Flush() : 613387 ms
Without cleanup + Flush() : 253331 ms
Extra overhead (Flush x N) : 360056 ms
Avg overhead per tablet : 360 ms
=== KUDU-1060 One-Time-Cost Benchmark ===
Tablets : 1000
Orphaned blocks / tablet : 1000000
Pass 1 - 1st restart (fix ON, dirty) : 629216 ms [cleanup+Flush for all tablets]
Pass 2 - 2nd restart (fix ON, clean) : 114 ms [superblocks already clean]
Pass 3 - clean baseline (fix OFF, clean) : 112 ms [no orphaned blocks to parse]
Pass 4 - legacy baseline (fix OFF, dirty) : 262499 ms [parses orphan IDs, no Flush]
Pass1 overhead vs Pass4 legacy : 366717 ms
Pass2 vs Pass3 clean baseline : 2 ms (should be ~0)
Change-Id: I732133b460df2fe4c91d05f420dda6f0274d440e
---
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_metrics.cc
M src/kudu/tablet/tablet_metrics.h
M src/kudu/tablet/tablet_mm_ops-test.cc
M src/kudu/tablet/tablet_mm_ops.cc
M src/kudu/tablet/tablet_mm_ops.h
10 files changed, 982 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/24125/2
--
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: newpatchset
Gerrit-Change-Id: I732133b460df2fe4c91d05f420dda6f0274d440e
Gerrit-Change-Number: 24125
Gerrit-PatchSet: 2
Gerrit-Owner: Ashwani Raina <ar...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [WIP] Add online gc op to clean orphaned blocks
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/24125 )
Change subject: [WIP] Add online gc op to clean orphaned blocks
......................................................................
Patch Set 1: Verified+1
Build Successful
http://jenkins.kudu.apache.org/job/pre_commit/2411/ : SUCCESS
--
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: 1
Gerrit-Owner: Ashwani Raina <ar...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 21 Mar 2026 15:04:41 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-829 Add online gc op to clean orphaned blocks
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/24125
to look at the new patch set (#4).
Change subject: KUDU-829 Add online gc op to clean orphaned blocks
......................................................................
KUDU-829 Add online gc op to clean orphaned blocks
This patch adds a background maintenance GC operation that makes use
of persisted information about set of orphaned blocks that could not be
deleted earlier due to transient errors, and tries to delete those in
the background. Following retain-on-failure semantics, failure paths
ensure the blocks (for which clean-up failed) are retained for a
subsequent retry from either GC, Flush, or superblock-load.
Main highlights:
- Add RetryOrphanedBlockDeletion as the main driving function that does
fetching of orphaned blocks, call DeleteOrphanedBlocks for the set of
orphaned blocks.
- Ensure DeleteOrphanedBlocks doesn't erase all the block entries from
the set of orphaned blocks when either partial or nothing from the set
was successfully deleted. The blocks with failed deletions are to be
used in next iteration by either GC or LoadFromSuperBlock or Flush.
- With this change, ensure when superblock load kicks in, we try to
delete the stale orphaned blocks only once. Irrespective of what the
outcome is from deletion, erase all the block entries from in-memory
set. This avoids possibility of those blocks being retried for delete
unnecessarily.
- LBM Restore Logic: Restore failed blocks to in-memory LBM state if
lower layer error was hit (maybe inside RemoveBlockIdsFromMetadata).
- Enhanced Metrics:
* Add `orphaned_blocks_count` gauge and cleanup path counters.
* Track cleanup by GC, flush, and load operations separately.
* Add failure counter for deletion attempts.
- Add unit tests to:
* Test that OrphanedGCOp runs when orphaned blocks are to be deleted.
* Test for retain-on-failure, and backoff logic.
* Cover both implementations: NativeMeta and RdbMeta.
* Add integration tests for all cleanup paths (GC, flush, load).
* Measure performance benchmark for the KUDU-1060 startup overhead.
* Test startup overhead with many tombstoned tablets.
- To address the issue of frequent scheduling of GC operation if there
are persistent I/O errors, add backoff logic that increments the time
duration exponentially after each repeated failure hit during cleaning
of orphaned blocks (possibly due to some persistent disk error). Until
this time duration passes, no orphaned blocks GC op is scheduled.
=== KUDU-1060 Startup Overhead Benchmark ===
Tablets : 1000
Orphaned blocks / tablet : 1000000
With cleanup + Flush() : 613387 ms
Without cleanup + Flush() : 253331 ms
Extra overhead (Flush x N) : 360056 ms
Avg overhead per tablet : 360 ms
=== KUDU-1060 One-Time-Cost Benchmark ===
Tablets : 1000
Orphaned blocks / tablet : 1000000
Pass 1 - 1st restart (fix ON, dirty) : 629216 ms [cleanup+Flush for all tablets]
Pass 2 - 2nd restart (fix ON, clean) : 114 ms [superblocks already clean]
Pass 3 - clean baseline (fix OFF, clean) : 112 ms [no orphaned blocks to parse]
Pass 4 - legacy baseline (fix OFF, dirty) : 262499 ms [parses orphan IDs, no Flush]
Pass1 overhead vs Pass4 legacy : 366717 ms
Pass2 vs Pass3 clean baseline : 2 ms (should be ~0)
TODO (to be addressed in separate patch/es):
- Investigate possibility of un-fsynced DELETE record lost on crash.
Explore the feasibility of syncing container metadata to ensure DELETE
records land in container before wirting clean superblock.
- Hitting WritableBlock::Abort() path after create record can also lead
to leaked space in blocks.
Change-Id: I732133b460df2fe4c91d05f420dda6f0274d440e
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_metrics.cc
M src/kudu/tablet/tablet_metrics.h
M src/kudu/tablet/tablet_mm_ops-test.cc
M src/kudu/tablet/tablet_mm_ops.cc
M src/kudu/tablet/tablet_mm_ops.h
12 files changed, 2,151 insertions(+), 108 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/24125/4
--
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: newpatchset
Gerrit-Change-Id: I732133b460df2fe4c91d05f420dda6f0274d440e
Gerrit-Change-Number: 24125
Gerrit-PatchSet: 4
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>
[kudu-CR] KUDU-829 Add online gc op to clean orphaned blocks
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins 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:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/2412/
--
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: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 23 Mar 2026 10:10:44 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-829 Add online gc op to clean orphaned blocks
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
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
[kudu-CR] KUDU-829 Add online gc op to clean orphaned blocks
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/24125
to look at the new patch set (#3).
Change subject: KUDU-829 Add online gc op to clean orphaned blocks
......................................................................
KUDU-829 Add online gc op to clean orphaned blocks
This patch adds a background maintenance GC operation that makes use
of persisted information about set of orphaned blocks that could not be
deleted earlier due to transient errors, and tries to delete those in
the background. Following retain-on-failure semantics, failure paths
ensure the blocks (for which clean-up failed) are retained for a
subsequent retry from either GC, Flush, or superblock-load.
Main highlights:
- Add RetryOrphanedBlockDeletion as the main driving function that does
fetching of orphaned blocks, call DeleteOrphanedBlocks for the set of
orphaned blocks.
- Ensure DeleteOrphanedBlocks doesn't erase all the block entries from
the set of orphaned blocks when either partial or nothing from the set
was successfully deleted. The blocks with failed deletions are to be
used in next iteration by either GC or LoadFromSuperBlock or Flush.
- With this change, ensure when superblock load kicks in, we try to
delete the stale orphaned blocks only once. Irrespective of what the
outcome is from deletion, erase all the block entries from in-memory
set. This avoids possibility of those blocks being retried for delete
unnecessarily.
- LBM Restore Logic: Restore failed blocks to in-memory LBM state if
lower layer error was hit (maybe inside RemoveBlockIdsFromMetadata).
- Enhanced Metrics:
* Add `orphaned_blocks_count` gauge and cleanup path counters.
* Track cleanup by GC, flush, and load operations separately.
* Add failure counter for deletion attempts.
- Add unit tests to:
* Test that OrphanedGCOp runs when orphaned blocks are to be deleted.
* Test for retain-on-failure, and backoff logic.
* Cover both implementations: NativeMeta and RdbMeta.
* Add integration tests for all cleanup paths (GC, flush, load).
* Measure performance benchmark for the KUDU-1060 startup overhead.
* Test startup overhead with many tombstoned tablets.
- To address the issue of frequent scheduling of GC operation if there
are persistent I/O errors, add backoff logic that increments the time
duration exponentially after each repeated failure hit during cleaning
of orphaned blocks (possibly due to some persistent disk error). Until
this time duration passes, no orphaned blocks GC op is scheduled.
=== KUDU-1060 Startup Overhead Benchmark ===
Tablets : 1000
Orphaned blocks / tablet : 1000000
With cleanup + Flush() : 613387 ms
Without cleanup + Flush() : 253331 ms
Extra overhead (Flush x N) : 360056 ms
Avg overhead per tablet : 360 ms
=== KUDU-1060 One-Time-Cost Benchmark ===
Tablets : 1000
Orphaned blocks / tablet : 1000000
Pass 1 - 1st restart (fix ON, dirty) : 629216 ms [cleanup+Flush for all tablets]
Pass 2 - 2nd restart (fix ON, clean) : 114 ms [superblocks already clean]
Pass 3 - clean baseline (fix OFF, clean) : 112 ms [no orphaned blocks to parse]
Pass 4 - legacy baseline (fix OFF, dirty) : 262499 ms [parses orphan IDs, no Flush]
Pass1 overhead vs Pass4 legacy : 366717 ms
Pass2 vs Pass3 clean baseline : 2 ms (should be ~0)
Change-Id: I732133b460df2fe4c91d05f420dda6f0274d440e
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_metrics.cc
M src/kudu/tablet/tablet_metrics.h
M src/kudu/tablet/tablet_mm_ops-test.cc
M src/kudu/tablet/tablet_mm_ops.cc
M src/kudu/tablet/tablet_mm_ops.h
12 files changed, 2,151 insertions(+), 108 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/24125/3
--
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: newpatchset
Gerrit-Change-Id: I732133b460df2fe4c91d05f420dda6f0274d440e
Gerrit-Change-Number: 24125
Gerrit-PatchSet: 3
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>