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.

Sign up for GitHub

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

Jump to bottom

connect dispatcher again on reset cursor complete #250

Merged
merged 3 commits into from
Feb 28, 2017
Merged

connect dispatcher again on reset cursor complete #250

merged 3 commits into from
Feb 28, 2017

Conversation

rdhabalia
Copy link
Contributor

Motivation

Reset-cursor disconnects consumers and closes Dispatcher that will not allow message-dispatching (because closeFuture!=null) even after reset-cursor completes.

Modifications

Reset cursor:

  • Disconnects dispatcher before reset cursor and connects dispatcher back once reset-cursor gets completed.

Result

Reset-cursor will not block message-dispatching for newly added consumers.

@rdhabalia rdhabalia added the type/bug The PR fixed a bug or issue reported a bug label Feb 27, 2017
@rdhabalia rdhabalia added this to the 1.17 milestone Feb 27, 2017
@rdhabalia rdhabalia self-assigned this Feb 27, 2017
@rdhabalia
Copy link
Contributor Author

@merlimat @saandrews we have to add it to the patch as regression tests are failing.

merlimat
merlimat reviewed Feb 27, 2017
View reviewed changes
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.

Just a few naming comments. If it's urgent just go ahead with current names

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/service/Dispatcher.java Outdated
* disconnect all consumers and mark dispatcher closed to stop new incoming requests
*
* @return
*/
CompletableFuture<Void> disconnect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should also rename this method to something like disconnectAllConsumers()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, updated name as : disconnectAllConsumers()

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/service/Dispatcher.java Outdated
/**
* mark dispatcher open to serve new incoming requests
*/
void connect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming sound a bit strange to me, since the the dispatcher itself does not "connect" to anything :)

What about reset() or something else?

...-broker/src/main/java/com/yahoo/pulsar/broker/service/persistent/PersistentSubscription.java Outdated
@@ -561,6 +560,11 @@ public double getExpiredMessageRate() {
return expiryMonitor.getMessageExpiryRate();
}

private void unfenceSubscriptionAndConnectDispatcher() {
Copy link
Contributor

Choose a reason for hiding this comment

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

naming is a bit confusing, what about resetState() or clearFencingState() ?

@rdhabalia
Copy link
Contributor Author

@merlimat updated changes.

@rdhabalia
Copy link
Contributor Author

@merlimat before merging this PR: was thinking another change:
introduce 2 methods:

  • disconnect() -> disconnect all consumers and disconnect dispatcher
  • disconnectAllConsumers() -> just disconnect all consumers
    and resetCursor() should call disconnectAllConsumers() and in that way, it doesn't have to dispatcher.reset()
    any thought?

@merlimat
Copy link
Contributor

  • disconnect() -> disconnect all consumers and disconnect dispatcher

Still I'm not clear what "disconnecting dispatcher" means :) . We can disconnect the consumer by either closing the TCP connection or anyway reject everything from it, but what does that mean for the dispatcher. Isn't then close() more appropriate?

@rdhabalia
Copy link
Contributor Author

Isn't then close() more appropriate?

Yes, I tried to explain as per old api disconnect(). But yes, it should be close() . so, it should have close() and disconnectAllConsumers()

@merlimat
Copy link
Contributor

👍

saandrews
saandrews reviewed Feb 27, 2017
View reviewed changes
pulsar-broker/src/test/java/com/yahoo/pulsar/client/impl/BrokerClientIntegrationTest.java Outdated
for (String subs : subList) {
log.info("got sub " + subs);
}
consumer.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Consumer close is not needed here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's correct. fixed it.

@rdhabalia rdhabalia force-pushed the reset_cursor branch 2 times, most recently from b36de9f to fb87bc8 Compare February 28, 2017 00:03
@rdhabalia
Copy link
Contributor Author

@merlimat updated discussed change. Can you please review it.

@saandrews
Copy link
Contributor

👍

saandrews
saandrews approved these changes Feb 28, 2017
View reviewed changes
Copy link
Contributor

@saandrews saandrews left a comment

Choose a reason for hiding this comment

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

Looks good

@rdhabalia rdhabalia merged commit d89563c into apache:master Feb 28, 2017
rdhabalia added a commit that referenced this pull request Feb 28, 2017
@rdhabalia
Disconnect consumers without closing dispatcher on cursor-reset (#250)
0337e61
@rdhabalia rdhabalia deleted the reset_cursor branch June 21, 2017 18:55
sijie pushed a commit to sijie/pulsar that referenced this pull request Mar 4, 2018
@srkukarni @sijie
Renamed all container stuff to runtime (apache#250)
0497368
@xiaotongwang1 xiaotongwang1 mentioned this pull request Aug 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saandrews saandrews saandrews approved these changes

@merlimat merlimat merlimat approved these changes

Assignees

@rdhabalia rdhabalia

Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
1.17
Development

Successfully merging this pull request may close these issues.
3 participants
@rdhabalia @merlimat @saandrews