-
Notifications
You must be signed in to change notification settings - Fork 695
Tpcc cli and import cleanup (#17333) #19771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tpcc cli and import cleanup (#17333) #19771
Conversation
🟢 |
⚪
*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
⚪
*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
🟢
*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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the TPCC CLI and runner by introducing duration-based options, unifying display logic, and cleaning up import and thread‐count calculations.
- Add a
Config
override for the TPCC import command and expose new CLI flags - Switch warmup/run times from integer minutes to
TDuration
parsing - Factor out display mode logic into
TRunConfig::SetDisplay()
and update runner/import to use it
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
ydb/public/lib/ydb_cli/commands/ydb_workload_tpcc.cpp | Added Config() for import, new CLI options (warehouses, threads) |
ydb/library/workload/tpcc/runner.h | Switched from minute-integers to TDuration , added format enum |
ydb/library/workload/tpcc/runner.cpp | Refactored display logic, updated thread-count heuristics, logging |
ydb/library/workload/tpcc/import.cpp | Replaced SetDisplayUpdateInterval with SetDisplay() , import fixes |
ydb/library/workload/tpcc/constants.h | Introduced WAREHOUSES_PER_CPU_CORE constant |
Comments suppressed due to low confidence (3)
ydb/public/lib/ydb_cli/commands/ydb_workload_tpcc.cpp:169
- The threads option is marked as required but has no
.DefaultValue()
, forcing users to always specify it. Consider adding.DefaultValue(RunConfig->ThreadCount)
to maintain consistency.
't', "time", TStringBuilder() << "Execution time. Example: 10s, 5m, 1h")
ydb/library/workload/tpcc/runner.cpp:166
- [nitpick] You added a new
DURATION
parsing option for warmup. Consider adding unit tests to validate parsing of inputs like "10s", "5m", and "1h".
void ExitTuiMode();
ydb/library/workload/tpcc/runner.cpp:486
- Replaced direct
std::cout
output withLOG_I()
, which may redirect text mode output into logs rather than stdout. This change could break existing CLI users who rely on console output; verify intended behavior.
LOG_I(ss.str());
Changelog entry
...
Changelog category
Description for reviewers
...