Feat[BMQ]: Add an admin command to remove a domain #541
+2,002
−37
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emelialei88
force-pushed
the
admin-domain-remove
branch
2 times, most recently
from
a7cb475 to
d760dbb
Compare
678098
reviewed
Dec 10, 2024
678098
reviewed
Dec 10, 2024
emelialei88
force-pushed
the
admin-domain-remove
branch
2 times, most recently
from
fcd0589 to
40eb300
Compare
emelialei88
force-pushed
the
admin-domain-remove
branch
4 times, most recently
from
035193e to
bc06ff4
Compare
emelialei88
force-pushed
the
admin-domain-remove
branch
4 times, most recently
from
434b216 to
12e54ee
Compare
emelialei88
force-pushed
the
admin-domain-remove
branch
4 times, most recently
from
3f686e0 to
bd5cec4
Compare
pniedzielski
requested changes
Jan 7, 2025
| @@ -73,6 +73,14 @@ struct CommandDefinition { | |||
| "Clear the domain resolution cache entry of the optionally specified " | |||
| "'domain', or clear all domain resolution cache entries if 'ALL' is " | |||
| "specified."}, | |||
| {"DOMAINS REMOVE <domain> [finalize]", | |||
| "Remove a domain with an optional keyword 'finalize'", | |||
There was a problem hiding this comment.
I think this description should reveal less of the implementation:
Remove a domain from the cluster. If the keyword 'FINALIZE' is not supplied, perform the first phase of domain deletion, failing if there is an open queue on this domain and blocking any open queue requests subsequent to the command being issued. After the first phase of domain deletion, it is safe to remove the domain configuration file from disk. If the keyword 'FINALIZE' is specified, perform the second phase of domain deletion, failing if the domain has not had the first phase of domain deletion performed. After the second phase of domain deletion, it is safe to create a new domain reusing the same name and to connect to it.
Open questions from the above:
- Your text here reads like it is undefined behavior if the second pass occurs without the first pass. If this is true, I think we should change it to just fail in that case.
- Reading this written out, I think we need to clean the domain resolver cache and config provider cache in the second phase, not the first. If someone connects to a domain that has been deleted in the first pass, doesn't the broker need to go through the domain resolver to know what
Domainobject to talk to, so it can find out the domain has been deleted and reject the request? There will still be a tiny race condition, but I think clearing these caches right before you delete the domain object is the correct behavior.
There was a problem hiding this comment.
- We do check the state of the domain to make sure we only continue the second round when the first is completed.
- Good catch!
|
|
||
| DomainSp domainSp; | ||
|
|
||
| if (0 != locateOrCreateDomain(&domainSp, name)) { |
There was a problem hiding this comment.
We talked about this, but just to record it down: the reason we do this here is due to the lazy loading of domains that BlazingMQ does. We don't want to fail if a user creates a domain configuration file, happens not to connect, and then issues this admin command, or creates a domain configuration file, uses the domain, purges the domain, and restarts the broker before issuing this admin command.
There was a problem hiding this comment.
But, this will load the domain into the resolver cache, so we do need to clear that on the second pass.
Signed-off-by: Emelia Lei <[email protected]>
implement the first pass where the config file of the domain is still on the disk, and the admin command can only be sent to leader/primary Signed-off-by: Emelia Lei <[email protected]>
Signed-off-by: Emelia Lei <[email protected]>
Remove the domain object in the second pass Signed-off-by: Emelia Lei <[email protected]>
…d pass. If an open queue request is issued right fter the first pass, the domain will be loaded into domainResolver and configProvider. We need to move cache cleaning to right before the remove of the domain object. Signed-off-by: Emelia Lei <[email protected]>
emelialei88
force-pushed
the
admin-domain-remove
branch
from
703efb4 to
9dcd5c1
Compare
|
I think we should add a test-case or two just to verify the change in that final commit. Then should be good to go. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The existing admin command "
DOMAINS DOMAIN <name> PURGE" only add purge record to the journal files for the queues belonging to this domain. This may cause race condition where it's possible for a user to connect to a queue that's purged.This PR adds a new admin command "
DOMAINS DOMAIN <name> REMOVE" to:domainResolverandconfigProviderWith a temporary second pass "
DOMAINS DOMAIN <name> REMOVE FINALIZE" to: