Skip to content

[Broker] no-persistent topic support terminate#11898

Closed
shibd wants to merge 1 commit intoapache:masterfrom
shibd:feat-11834
Closed

[Broker] no-persistent topic support terminate#11898
shibd wants to merge 1 commit intoapache:masterfrom
shibd:feat-11834

Conversation

@shibd
Copy link
Member

@shibd shibd commented Sep 2, 2021

Copy link
Contributor

@congbobo184 congbobo184 left a comment

Choose a reason for hiding this comment

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

Could you please add a test for it?

@Override
public CompletableFuture<MessageId> terminate() {
CompletableFuture<MessageId> future = new CompletableFuture<>();
producers.values().forEach(Producer::disconnect);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not disconnect sub?

Copy link
Member Author

@shibd shibd Sep 2, 2021

Choose a reason for hiding this comment

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

Thanks for the review. I think still consumer to drain existing messages in backlog after the termination.Disconnect subscribe will close existing consumers.

May need to call Subscription#close() that no longer accepts new consumers

    @Override
    public CompletableFuture<Void> close() {
        IS_FENCED_UPDATER.set(this, TRUE);
        return CompletableFuture.completedFuture(null);
    }

Do you have any better suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need retain existing consumer when non-persistent topic terminate? @gaoran10 @codelipenghui /cc

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 non-persistent termination should behave like persistent

@shibd
Copy link
Member Author

shibd commented Sep 2, 2021

Could you please add a test for it?

Ok, I'll add it

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

The problem with non-persistent topics is that the termination will be lost at the next broker restart or topic failover so it would not make much practical sense.

@shibd
Copy link
Member Author

shibd commented Sep 2, 2021

The problem with non-persistent topics is that the termination will be lost at the next broker restart or topic failover so it would not make much practical sense.

Got it, I will close this pr.

@shibd shibd closed this Sep 2, 2021
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.

4 participants