Tpcc import imrovements (#17333) #19791
Conversation
|
🟢 |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the TPCC import tool by tightening CLI option requirements, improving interrupt- and retry-handling in the loader, and adding sanity assertions to data-splitting unit tests.
- Require
--warehouses(and hide--log-level) in thetpcc init,import, andruncommands - Add global interrupt checks and more granular retry-logic in
ExecuteWithRetryandTPCCLoader - Strengthen unit tests with non-negative/assert‐positive split counts
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ydb/public/lib/ydb_cli/commands/ydb_workload_tpcc.cpp | Made Config overrides non-virtual, required warehouse arg |
| ydb/library/workload/tpcc/ut/data_splitter_ut.cpp | Added UNIT_ASSERT checks for non-negative split counts |
| ydb/library/workload/tpcc/import.cpp | Switched to global interrupt source; refined retry logging |
Comments suppressed due to low confidence (1)
ydb/public/lib/ydb_cli/commands/ydb_workload_tpcc.cpp:112
- The
tpcc importandtpcc runcommands now require--warehouses, but onlytpcc inithas a hidden--log-leveloption. For consistency, consider adding the--log-levelflag to theimportandruncommand configurations as well.
'w', "warehouses", TStringBuilder() << "Number of warehouses")
| @@ -73,6 +74,7 @@ Y_UNIT_TEST_SUITE(TDataSplitterTest) { | |||
| int stockWarehousesPerShard2 = (1000 + minShardCount - 1) / minShardCount; | |||
| stockWarehousesPerShard = std::min(stockWarehousesPerShard, stockWarehousesPerShard2); | |||
| int expectedStockSplits = (1000 - 1) / stockWarehousesPerShard; | |||
| UNIT_ASSERT(expectedStockSplits > 0); | |||
There was a problem hiding this comment.
The first assertion uses >= 0 but here you use > 0, which is inconsistent and may fail if zero splits are valid. Consider standardizing on one boundary check (e.g. >= 0) or adding a comment explaining why zero is invalid.
| UNIT_ASSERT(expectedStockSplits > 0); | |
| UNIT_ASSERT(expectedStockSplits >= 0); // Ensure consistency with earlier assertions |
Copilot uses AI. Check for mistakes.
| @@ -511,15 +511,28 @@ NTable::TBulkUpsertResult LoadOrderLines( | |||
| template<typename LoadFunc> | |||
| void ExecuteWithRetry(const TString& operationName, LoadFunc loadFunc, TLog* Log) { | |||
| for (int retryCount = 0; retryCount < MAX_RETRIES; ++retryCount) { | |||
| if (GetGlobalInterruptSource().stop_requested()) { | |||
| break; | |||
There was a problem hiding this comment.
[nitpick] Breaking out of the retry loop on interrupt will naturally exit the function, but using return; instead of break; makes the intent explicit and avoids confusion about control flow.
| break; | |
| return; |
Copilot uses AI. Check for mistakes.
Changelog entry
...
Changelog category
Description for reviewers
...