-
Notifications
You must be signed in to change notification settings - Fork 520
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
Replace Icb with ExperimentingService entity. #6661
base: main
Are you sure you want to change the base?
Conversation
Hi! Thank you for contributing! |
06aa515
to
4eaf1e8
Compare
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.
Пока совсем не то что нужно
ydb/core/base/appdata.h
Outdated
struct TExperimentingService : public TThrRefBase { | ||
#define EXP_SERVICE_REG_SHARED(expService, name, value) \ | ||
if (!(expService).name.IsDefined()) { \ | ||
(expService).name = value; \ |
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.
параметры макросов нужно обязательно брать в скобки, иначе при подстановке может возникнуть какой-нибудь ужасный ужас, например если засунуть в макрос
int var = 3;
#define MACRO(value) var = value;
int main() {
int y = 0;
MACRO(0; y=5);
return y;
}
Скобки не панацея но лучше чем ничего
ydb/core/base/appdata.h
Outdated
value = (expService).name; \ | ||
} | ||
|
||
#define EXP_SERVICE_REG_LOCAL(expService, name, value) \ |
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.
Вообще, важнее чем вопрос скобок - вопрос, а зачем вообще тут нужны макросы?
Почему это сделано так, а не в виде метода нового класса
TMyWrapper DataShardControlsDisableByKeyFilter;
или производного от TControlWrapper например?
Почему не переместить назначение дефолтного значения прямо в конструктор?
вот так:
TMyWrapper DataShardControlsDisableByKeyFilter(100500);
Так вообще не нужным станет
#define EXP_SERVICE_REG_SHARED
ydb/core/base/appdata.h
Outdated
TControlWrapper SchemeShardAllowDataColumnForIndexTable{nullptr}; | ||
TControlWrapper SchemeShardAllowServerlessStorageBilling{nullptr}; | ||
}; | ||
|
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.
Главное чего тут не случилось - тут нет метода, позволяющего получить по имени элемент, это означает, что отвалится централизованное управление кластером. То есть то ради чего все и нужно.
f0626d4
to
1e96993
Compare
Replace Immediate Controls Board (ICB) with ExperimentingService entity.
Remove unnecessary synchronization.