Skip to content
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

fix(namespace): didn't reload namespaces from DB after the full sync #2527

Merged
merged 2 commits into from
Sep 8, 2024

Conversation

git-hulk
Copy link
Member

@git-hulk git-hulk commented Sep 8, 2024

Namespaces would store inside DB if the repl-namespace-enabled was set to yes.
Then replicas will intercept and parse the increment replication log to
see if it needs to reload namespaces. But it won't have the namespace update log
when doing the full sync, so we need to reload from DB once the full replication is done.

This closes #2520

Namespaces will store inside DB if the repl-namespace-enabled was set to
yes. And then replicas will intercept and parse the increment replication log
to see if it needs to reload namespaces. But it won't have the namespace
update log when doing the full sync, so we need to forcely reload from
DB once the full replication was done.
@git-hulk git-hulk changed the title Fix didn't reload namespaces from DB after the full sync fix(namespace): didn't reload namespaces from DB after the full sync Sep 8, 2024
if (!s.IsOK()) {
LOG(ERROR) << "[replication] Failed to load and rewrite namespace: " << s.Msg();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract a PostFullSync function, move it to this function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have post_fullsync_cb now, but it's used for cleaning resources if failed to do the full sync. So it should be not good to put this inside that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it a callback function after successfully loading the data?

auto s = replication_thread_->Start([this]() { PrepareRestoreDB(); },
[this]() {
this->is_loading_ = false;
if (auto s = task_runner_.Start(); !s) {
LOG(WARNING) << "Failed to start task runner: " << s.Msg();
}
});

Copy link
Member Author

@git-hulk git-hulk Sep 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No indeed, it should be used for resetting resources corresponding to pre_fullsync_cb. That said, it must be called once if the pre_fullsync_cb was invoked. I will submit another PR to fix #2511 which is caused by that didn't call post_fullsync_cb while failing to restore DB.

@mapleFU
Copy link
Member

mapleFU commented Sep 8, 2024

But it won't have the namespace update log when doing the full sync, so we need to reload from DB once the full replication is done.

Can you point out the reason here?

@git-hulk
Copy link
Member Author

git-hulk commented Sep 8, 2024

But it won't have the namespace update log when doing the full sync, so we need to reload from DB once the full replication is done.

Can you point out the reason here?

What's the meaning of this comment? Do you mean why the full sync won't propagate the namespace change log?

@mapleFU
Copy link
Member

mapleFU commented Sep 8, 2024

Do you mean why the full sync won't propagate the namespace change log?

Yes I'm learning the code here, it would help if you can point the reason out

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I got to know this, since #1776 do not reload when ns is added...

@git-hulk git-hulk merged commit a29df09 into apache:unstable Sep 8, 2024
31 checks passed
Copy link

sonarcloud bot commented Sep 8, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Didn't load namespaces into memory after full syncing the db from master node
4 participants