Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24901 )
Change subject: KUDU-2874 Add option to allow huge cells. ...................................................................... Patch Set 3: (3 comments) Couple observations from my end. Test coverage gaps: the new restart test and the un-DISABLED reproducer are good, but I think it would be good to see coverage for: - Upper-bound enforcement: a cell larger than max_huge_cell_size_bytes is rejected even when allow_huge is set (and that a non-huge table still rejects at max_cell_size_bytes). This is the core new limit and is currently untested. - Config round-trip / parse: ParseAllowHugeConfig — valid "auto_batch_size", invalid value returns InvalidArgument, and ExtraConfigPBFromPBMap <-> ExtraConfigPBToPBMap round-trips the kudu.table.allow_huge key. The disable_compaction pattern in alter_table-test.cc:2732 is a good template. (These are small, cheap unit tests) http://gerrit.cloudera.org:8080/#/c/24901/3/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/24901/3/src/kudu/tablet/tablet.cc@1475 PS3, Line 1475: switch (decoded.type) { The switch only accounts for INSERT/INSERT_IGNORE/UPSERT/UPSERT_IGNORE (UPDATE is a documented TODO). But with allow_huge set, the decoder's per-cell limit is raised to max_huge_cell_size_bytes for all ops including UPDATE (DecodeWriteOperations), so an UPDATE can grow a string cell to ~1MB without ever lowering batch_size_override. An ordered scan over such a tablet would still hit the original OOM this patch is meant to prevent. Is that combination reachable in practice, and if so should the limitation be called out more loudly than an inline TODO? http://gerrit.cloudera.org:8080/#/c/24901/3/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: http://gerrit.cloudera.org:8080/#/c/24901/3/src/kudu/tablet/tablet_metadata.cc@1047 PS3, Line 1047: bool TabletMetadata::allows_huge_cells() const { Nothing in this patch prevents allow_huge from being reset to NOT_ALLOWED (it's just extra-config). If a tablet already holds 1MB cells and the property is cleared, allows_huge_cells() returns false, scans revert to the 128-row default, and we're back to the OOM scenario. Is disabling validated/blocked on the master side (that integration isn't in this patch - the test even notes "this should be a table and not a tablet-level setting"), or is that a follow-up? Worth a note on the intended guardrails. http://gerrit.cloudera.org:8080/#/c/24901/3/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/24901/3/src/kudu/tserver/tablet_service.cc@3242 PS3, Line 3242: if (tablet_metadata->allows_huge_cells()) { allows_huge_cells() takes data_lock_ (a simple_spinlock, see tablet_metadata.cc:1048), and HandleContinueScanRequest runs once per batch continuation for every scan on every table, not just huge-cell tables. The commit message says a non-huge tablet only pays "a quick check for the attribute," but this is now a lock acquisition - the same lock held during metadata/superblock flush. Could you gate the common case behind a lock-free read? Since batch_size_override_ is already std::atomic, an std::atomic<bool> allows_huge_ cache (set in LoadFromSuperBlock/SetExtraConfig) would keep the default path lock-free and match the stated design. Same applies to the allows_huge_cells() call in tablet.cc Iterator::Init (less hot, but same idea). -- To view, visit http://gerrit.cloudera.org:8080/24901 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8a736933cbe58b7a174fac629b0e8b3d3bd72c18 Gerrit-Change-Number: 24901 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Martonka <[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-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Tue, 22 Sep 2026 14:42:03 +0000 Gerrit-HasComments: Yes
