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

[broker] Make resetting cursor in REST API asynchronous #7744

Merged
merged 3 commits into from
Aug 6, 2020

Conversation

massakam
Copy link
Contributor

@massakam massakam commented Aug 4, 2020

Motivation

As mentioned in #7478, PersistentSubscription#resetCursor() may not complete for unknown reasons. This blocks pulsar-web threads, so it can cause the broker server to be unable to respond to HTTP requests.

Modifications

Get the result of PersistentSubscription#resetCursor() asynchronously so as not to block pulsar-web threads.

@massakam massakam added type/bug The PR fixed a bug or issue reported a bug area/broker release/2.6.2 labels Aug 4, 2020
@massakam massakam added this to the 2.7.0 milestone Aug 4, 2020
@massakam massakam self-assigned this Aug 4, 2020
Copy link
Contributor

@nkurihar nkurihar left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -232,7 +232,6 @@ public void testTerminatePartitionedTopic() {

@Test
public void testNonPartitionedTopics() {
pulsar.getConfiguration().setAllowAutoTopicCreation(false);
Copy link
Contributor Author

@massakam massakam Aug 5, 2020

Choose a reason for hiding this comment

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

This test fails if allowAutoTopicCreation is false.

We are testing if the subscription creation is successful here. However, if allowAutoTopicCreation is false, the topic cannot be created and the subscription creation should also fail.

boolean isAllowAutoTopicCreation = pulsar().getConfiguration().isAllowAutoTopicCreation();
PersistentTopic topic = (PersistentTopic) pulsar().getBrokerService()
.getTopic(topicName.toString(), isAllowAutoTopicCreation).thenApply(Optional::get).join();

I think it's a mistake to set allowAutoTopicCreation to false here. Previously this test passed because the REST API was not able to return the error response correctly.

@sijie sijie merged commit 69cbd26 into apache:master Aug 6, 2020
@massakam massakam deleted the reset-cursor-async branch August 6, 2020 15:43
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
### Motivation

As mentioned in apache#7478, `PersistentSubscription#resetCursor()` may not complete for unknown reasons. This blocks `pulsar-web` threads, so it can cause the broker server to be unable to respond to HTTP requests.

### Modifications

Get the result of `PersistentSubscription#resetCursor()` asynchronously so as not to block `pulsar-web` threads.
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
### Motivation

As mentioned in apache#7478, `PersistentSubscription#resetCursor()` may not complete for unknown reasons. This blocks `pulsar-web` threads, so it can cause the broker server to be unable to respond to HTTP requests.

### Modifications

Get the result of `PersistentSubscription#resetCursor()` asynchronously so as not to block `pulsar-web` threads.
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
### Motivation

As mentioned in apache#7478, `PersistentSubscription#resetCursor()` may not complete for unknown reasons. This blocks `pulsar-web` threads, so it can cause the broker server to be unable to respond to HTTP requests.

### Modifications

Get the result of `PersistentSubscription#resetCursor()` asynchronously so as not to block `pulsar-web` threads.
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
### Motivation

As mentioned in apache#7478, `PersistentSubscription#resetCursor()` may not complete for unknown reasons. This blocks `pulsar-web` threads, so it can cause the broker server to be unable to respond to HTTP requests.

### Modifications

Get the result of `PersistentSubscription#resetCursor()` asynchronously so as not to block `pulsar-web` threads.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker release/2.6.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants