Add ChainActor ping kind #12917
Add ChainActor ping kind #12917
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 |
| {} | ||
|
|
||
| TCommandLatency::~TCommandLatency() { | ||
| delete ChainConfig; |
There was a problem hiding this comment.
Otherwise I either need to include everything in the header or to rewrite the class using pimpl. Both ways are ugly in this particular case, imho, while this one is slightly less ugly, but fast.
There was a problem hiding this comment.
namespace NDebug {
class TActorChainPingSettings;
}
...
std:unique_ptr<NDebug::TActorChainPingSettings> ChainConfig;There was a problem hiding this comment.
I did it this way :) But I made a mistake and didn't realize that I needed to define the destructor myself, not use the default one, where the definition of NDebug::TActorChainPingSettingsexists.
| @@ -57,6 +65,12 @@ struct TKqpProxyPingSettings : public TOperationRequestSettings<TKqpProxyPingSet | |||
| struct TSchemeCachePingSettings : public TOperationRequestSettings<TSchemeCachePingSettings> {}; | |||
| struct TTxProxyPingSettings : public TOperationRequestSettings<TTxProxyPingSettings> {}; | |||
|
|
|||
| struct TActorChainPingSettings : public TOperationRequestSettings<TActorChainPingSettings> { | |||
| size_t ChainLength = 10; | |||
There was a problem hiding this comment.
I prefer simple struct with direct member access in a such simple cases.
There was a problem hiding this comment.
It is not a common way in cpp sdk, especially for *Settings structures.
eivanov89
force-pushed
the
ydb-chainactor-ping
branch
from
8885d5c to
0dbc726
Compare
|
⚪ 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 |
|
⚪ 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 |
| "chain-length", TStringBuilder() << "Chain length (ActorChain kind only)") | ||
| .OptionalArgument("INT").StoreResult(&ChainConfig->ChainLength).DefaultValue(ChainConfig->ChainLength); | ||
| config.Opts->AddLongOption( | ||
| "chain-work-usec", TStringBuilder() << "Amount of work for each actor in the chain (ActorChain kind only)") |
There was a problem hiding this comment.
Better don't use "usec" in option name. Leave it in the description section as a default. I hope in the nearest future there will be an opportunity to use "1ms", "3s" e.t.c for options with time intervals
| .OptionalArgument("STRING").StoreResult(&RunKind).DefaultValue(DEFAULT_RUN_KIND); | ||
|
|
||
| // actor chain options | ||
| config.Opts->AddLongOption( |
There was a problem hiding this comment.
New CLI options should be addressed in changelog.md
eivanov89
force-pushed
the
ydb-chainactor-ping
branch
from
0dbc726 to
8c0ae08
Compare
eivanov89
force-pushed
the
ydb-chainactor-ping
branch
from
8c0ae08 to
21b2dbf
Compare
Co-authored-by: Nikolay Perfilov <[email protected]>
eivanov89
force-pushed
the
ydb-chainactor-ping
branch
from
7210953 to
742de0d
Compare
|
⚪ 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 |
|
⚪ 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 |
| @@ -57,6 +65,12 @@ struct TKqpProxyPingSettings : public TOperationRequestSettings<TKqpProxyPingSet | |||
| struct TSchemeCachePingSettings : public TOperationRequestSettings<TSchemeCachePingSettings> {}; | |||
| struct TTxProxyPingSettings : public TOperationRequestSettings<TTxProxyPingSettings> {}; | |||
|
|
|||
| struct TActorChainPingSettings : public TOperationRequestSettings<TActorChainPingSettings> { | |||
| FLUENT_SETTING_DEFAULT(size_t, ChainLength, 10); | |||
| FLUENT_SETTING_DEFAULT(size_t, WorkUsec, 5); | |||
There was a problem hiding this comment.
Это время? Не надо использовать size_t для такого, тут надо std::chrono::microseconds.
Также сокращение Usec не очень писать тогда, по типу и так понятно будет. И на что влияет эта опция, по названию не очень понятно
There was a problem hiding this comment.
I would argue, that size_t in this particular place is absolutely fine. However, I agree that naming could be better.
There was a problem hiding this comment.
std::chrono it's a modern and type-safe approach. Why do you think that size_t is better to use here?
|
⚪ 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 |
|
⚪ 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 |
Changelog entry
Added new
ydb debug latencykindActorChain.Changelog category
Additional information
...