Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[improve][broker] PIP-192: Support delete namespace bundle admin API #19851

Conversation

Demogorgon314
Copy link
Member

PIP: #16691

Motivation

Raising a PR to implement #16691.

We need to support delete namespace bundle admin API.

Modifications

  • Support delete namespace bundle admin API.
  • Add units test.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@Demogorgon314 Demogorgon314 self-assigned this Mar 17, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 17, 2023
@Demogorgon314 Demogorgon314 force-pushed the Demogorgon314/PIP-192-support-namespace-bundle-delete branch from ade32bd to bb0998f Compare March 18, 2023 10:16
@Demogorgon314 Demogorgon314 force-pushed the Demogorgon314/PIP-192-support-namespace-bundle-delete branch from bb0998f to 1638aae Compare March 20, 2023 01:10
@Demogorgon314 Demogorgon314 marked this pull request as ready for review March 20, 2023 01:57
public CompletableFuture<Boolean> checkOwnershipPresentAsync(NamespaceBundle bundle) {
if (ExtensibleLoadManagerImpl.isLoadManagerExtensionEnabled(config)) {
ExtensibleLoadManagerImpl extensibleLoadManager = ExtensibleLoadManagerImpl.get(loadManager.get());
return extensibleLoadManager.getOwnershipAsync(Optional.empty(), bundle)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we pass Optional.empty() for system topics like bsc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you explain in detail?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't pass the Optional<ServiceUnitId> topic, I think the ownership cannot be found for the load manager system topics (like bsc, broker-load, top-bundles-load topics). If this checkOwnershipPresentAsync is not used to check the load manager system topics, we are good here.

// example how we use Optional<ServiceUnitId> topic
private CompletableFuture<Optional<String>> getOwnershipAsync(Optional<ServiceUnitId> topic,
                                                                 ServiceUnitId bundleUnit) {
        final String bundle = bundleUnit.toString();
        CompletableFuture<Optional<String>> owner;
        if (topic.isPresent() && isInternalTopic(topic.get().toString())) {
            owner = serviceUnitStateChannel.getChannelOwnerAsync();
        } else {
            owner = serviceUnitStateChannel.getOwnerAsync(bundle);
        }
        return owner;

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think when using checkOwnershipPresentAsync method, we can get the topic info.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove Optional<ServiceUnitId> topic and pass just bundleUnit only to getOwnershipAsync, like the one below. Let the leader own all system namespace topics, and we don't auto-unload system topics.

private CompletableFuture<Optional<String>> getOwnershipAsync(ServiceUnitId bundleUnit) {
        final String bundle = bundleUnit.toString();
        CompletableFuture<Optional<String>> owner;
        if (isSystemNamespace(bundleUnit.toString())) {
            owner = serviceUnitStateChannel.getChannelOwnerAsync();
        } else {
            owner = serviceUnitStateChannel.getOwnerAsync(bundle);
        }
        return owner;

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, adding another namespace will be better. Also, we shouldn't let users delete the system namespace, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think users need to delete system topics.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, then checkOwnershipPresentAsync will not be used for the system topics.

Copy link
Contributor

Choose a reason for hiding this comment

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

Plz throw NotSupportedOperationException if the input bundle's namespace is the system namespace .

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@Demogorgon314 Demogorgon314 added ready-to-test type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker labels Mar 22, 2023
@Technoboy- Technoboy- added this to the 3.0.0 milestone Mar 27, 2023
@Technoboy- Technoboy- closed this Mar 27, 2023
@Technoboy- Technoboy- reopened this Mar 27, 2023
@Demogorgon314 Demogorgon314 merged commit eedf702 into apache:master Mar 29, 2023
@Demogorgon314 Demogorgon314 deleted the Demogorgon314/PIP-192-support-namespace-bundle-delete branch March 29, 2023 02:23
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area/broker doc-not-needed Your PR changes do not impact docs ready-to-test type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants