Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24615 )
Change subject: [tablet] Skip stable_sort for already sorted batch ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/24615/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24615/2//COMMIT_MSG@31 PS2, Line 31: > To mimic OLAP bulk ingest with production of large batches with proportiona If targeting a custom environment with a lot of rows per RPC batch, I'd customize the mutation buffer space (default is 7MiB in the client library and 4MiB in the 'kudu perf loadgen' CLI tool) via the --buffer_size_bytes flag up to something close to the default RPC size limit (~50MiB) along with setting --buffer_flush_watermark_pct up to 1. If you want to go with higher numbers, you'd need to customize --rpc_max_message_size as well. By setting --buffer_flush_watermark_pct=1.0 you are getting only 4MiB of buffered operations. http://gerrit.cloudera.org:8080/#/c/24615/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24615/3//COMMIT_MSG@31 PS3, Line 31: --run_scan=false nit: it seems I missed it in prior review pass, but this flag doesn't require customization since it's 'false' by default http://gerrit.cloudera.org:8080/#/c/24615/3/src/kudu/tablet/tablet-test.cc File src/kudu/tablet/tablet-test.cc: http://gerrit.cloudera.org:8080/#/c/24615/3/src/kudu/tablet/tablet-test.cc@1832 PS3, Line 1832: EXPECT_EQ(pre_sorted_before + 1, PreSortedCount()); : EXPECT_EQ(needed_sort_before, NeededSortCount()); nit for here and elsewhere: any particular reason to use EXPECT_xxx instead of ASSERT_xxx? In other words, why to continue with the rest of a scenario if one of EXPECT_xxx triggers? http://gerrit.cloudera.org:8080/#/c/24615/2/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/24615/2/src/kudu/tablet/tablet.cc@1175 PS2, Line 1175: is_sorted = false; : } : keys_and_indexes.emplace_back(key, i); : } > The loop already exists here to build keys_and_indexes, adding is_sorted lo Thanks a lot for performing the benchmarking! Right: std::is_sorted() does short-circuiting when finding first out-of-order element as well. In approach #2, there is no need to check for is_sorted && !keys_and_indexes.empty() for every element, but perhaps, more important piece in play is memory access and cache line invalidation. http://gerrit.cloudera.org:8080/#/c/24615/3/src/kudu/tablet/tablet_metrics.h File src/kudu/tablet/tablet_metrics.h: http://gerrit.cloudera.org:8080/#/c/24615/3/src/kudu/tablet/tablet_metrics.h@78 PS3, Line 78: // Fast-path / slow-path split for the sort-skip optimisation in : // Tablet::BulkCheckPresence. : scoped_refptr<Counter> bulk_check_batches_pre_sorted; : scoped_refptr<Counter> bulk_check_batches_needed_sort; Reiterating on my question at https://gerrit.cloudera.org/#/c/24615/2/src/kudu/tablet/tablet.cc@1211: What purpose do these metrics serve beyond benchmarking with this changelist and tests? I'm interested to know what troubleshooting or debugging use case they might help with. Maybe, you have something particular where you find these metrics crucial to have? -- To view, visit http://gerrit.cloudera.org:8080/24615 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28d3f5ebdf7e3f13f04ffe8d35417a98c370fcf7 Gerrit-Change-Number: 24615 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: Wed, 02 Sep 2026 06:37:49 +0000 Gerrit-HasComments: Yes
