Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24452 )
Change subject: KUDU-3783 Extend clean-superblock guarantee into LoadFromSuperBlock ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/24452/3/src/kudu/tablet/tablet_metadata-test.cc File src/kudu/tablet/tablet_metadata-test.cc: http://gerrit.cloudera.org:8080/#/c/24452/3/src/kudu/tablet/tablet_metadata-test.cc@999 PS3, Line 999: const int64_t threshold = std::max<int64_t>(pass3_clean_baseline_ms * 1.2, 50); The threshold is `max(pass3 * 1.2, 50ms)`. This is a timing-based assertion in a unit test. I see your point from the local development verification side. Do you think this is reliable on dist-test/on test machines with load? http://gerrit.cloudera.org:8080/#/c/24452/3/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: http://gerrit.cloudera.org:8080/#/c/24452/3/src/kudu/tablet/tablet_metadata.cc@557 PS3, Line 557: if (!fs_manager()->read_only() && !orphaned_blocks.empty() && The old code called `DeleteOrphanedBlocks(orphaned_blocks)` unconditionally when `!read_only()`, regardless of whether the list was empty (the function early-returns on empty). With the new condition `!orphaned_blocks.empty()`, the behavior is unchanged for the empty case. However, note that the old code also ran even when `FLAGS_enable_tablet_orphaned_block_deletion == false` — it would just log a warning and return from inside `DeleteOrphanedBlocks`. With the new gating on `FLAGS_enable_orphaned_block_cleanup_on_load`, are these two flags intended to be orthogonal? If `enable_tablet_orphaned_block_deletion=false` AND `enable_orphaned_block_cleanup_on_load=true`, the code will call `DeleteOrphanedBlocks` (which does nothing) and then `Flush()` — persisting an empty orphaned_blocks_ set to disk, even though the blocks weren't actually deleted. Is that intentional? It seems like you'd want to check `enable_tablet_orphaned_block_deletion` here too, or at least document the interaction. http://gerrit.cloudera.org:8080/#/c/24452/3/src/kudu/tablet/tablet_metadata.cc@560 PS3, Line 560: DeleteOrphanedBlocks(orphaned_blocks); `DeleteOrphanedBlocks` unconditionally erases all entries from `orphaned_blocks_` even when the actual block deletion fails (see lines 637-640: "Regardless of whether we deleted all the blocks or not, remove them from the orphaned blocks list"). So if some blocks fail to delete (disk error), they're removed from the in-memory set, and then `Flush()` writes a "clean" superblock — but the blocks are still on disk. The next restart won't retry because the superblock no longer lists them. This is the pre-existing behavior for the normal-path cleanup, so it's arguably fine for load-time too. But given this patch's goal of "clean superblock guarantee", it's worth noting in the commit message or a comment that the guarantee is "superblock is clean" not "all orphaned blocks are definitely gone from disk." http://gerrit.cloudera.org:8080/#/c/24452/3/src/kudu/tablet/tablet_metadata.cc@565 PS3, Line 565: LOG_WITH_PREFIX(INFO) << Substitute( The INFO log fires after every load-time cleanup. For a TS with thousands of tombstoned tablets that all crashed with dirty superblocks, this produces one INFO line per tablet on the first restart. Consider VLOG(1) instead, or batching into a single summary at the end of bootstrap. Not a blocker - just something to think about for noisy startup logs. -- To view, visit http://gerrit.cloudera.org:8080/24452 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I729395085fb845daa667b1374dc1ac4291a0c30b Gerrit-Change-Number: 24452 Gerrit-PatchSet: 3 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Tue, 23 Jun 2026 12:44:32 +0000 Gerrit-HasComments: Yes
