-
Notifications
You must be signed in to change notification settings - Fork 586
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
Commit for autopartitioned topics #11629
base: main
Are you sure you want to change the base?
Commit for autopartitioned topics #11629
Conversation
⚪ |
⚪ |
⚪ |
⚪ |
⚪ |
⚪ |
} | ||
|
||
void TConsumerOperations::AddOperation(const TString& consumer, const Ydb::Topic::OffsetsRange& range) | ||
bool TConsumerOperations::GetOnlyCheckCommitedToFinish() const |
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.
почему Only? разве нельзя закомитить позицию по родительской партиции и заодно проверить этот предикат и отправить его в дочернюю?
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/kqp/topics/kqp_topics.cpp
Outdated
const NKikimrKqp::TTopicOperationsRequest_TopicOffsets_PartitionOffsets_OffsetsRange& range, | ||
bool forceCommit, | ||
bool killReadSession, | ||
bool onlyCheckCommitedToFinish) |
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.
CommittedToFinish...у нас разве так назвается? FullyProcesed может? Это значит и комит по конец, и факт закрытия партиции на запись
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.
В доке так, да. CommittedToFinishAndClosed может тогда? Длинное, зато понятное
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.
CommittedToEndOfClosed
ydb/core/persqueue/partition.cpp
Outdated
ok = false; | ||
} | ||
} else { | ||
if (!operation.GetForceCommit() && operation.GetCommitOffsetsBegin() > operation.GetCommitOffsetsEnd()) { // savnik check default |
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.
HasForceCommit не проверяешь. Выше проверяешь и Has. Может и выше не надо Has?
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.
чекну
|
||
userInfo.Offset = operation.GetEnd(); | ||
if (operation.GetKillReadSession()) { // savnik check default | ||
userInfo.Session = ""; |
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.
в точечном коммите до твоих изменений только это происходит?
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.
И важно это делать после персиста, а не при обработке запроса. ТУт еще предикаты от соседей не получены, не факт что транзакцию закомитят
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.
да, в точечном так же.
По поводу "после персиста" - смотрю
commits.push_back(commit); | ||
} | ||
|
||
TKqpHelper::TCommitInfo commit {.PartitionId = partitionNode->Id, .Offset = commitRequest->Getoffset()}; |
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.
а всякие там проверки не надо делать? Что там докомичен родитель до конца? Пусть будет полноценная операция, с проверками тоже!
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.
А зачем она тут? Это точечный коммит. Тут же можно ничего не проверять, только оффсеты правильно расставить в поддереве и у родителей. Или я не понял
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.
Можно не проверять. Если бага нет в формировании запроса. А если есть? Если ты про EndOffset неправильно узнал?
⚪ 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
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
if (record.GetYdbStatus() != Ydb::StatusIds::SUCCESS) { // savnik finish this part! | ||
Ydb::Topic::CommitOffsetResult result; | ||
Request().SendResult(result, record.GetYdbStatus()); | ||
Die(ctx); |
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.
return
if (step == TKqpHelper::ECurrentStep::DONE) { | ||
Ydb::Topic::CommitOffsetResult result; | ||
Request().SendResult(result, Ydb::StatusIds::SUCCESS); | ||
Die(ctx); |
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.
return
⚪ 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
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
d5a05e9
to
c9241c4
Compare
c9241c4
to
50195e2
Compare
⚪ |
⚪ |
Changelog entry
Changelog category
Additional information
...