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

Async authorization check while creation of producer/consumer #98

Merged
merged 1 commit into from
Nov 9, 2016

Conversation

rdhabalia
Copy link
Contributor

Motivation

While creating producer/consumer at broker: broker performs sync authorization check (sync zk-policies checks) on client-connection which blocks broker-io threads.

Modifications

Made async-authorization check while creating producer/consumer at broker and websocket server.

Result

No functional change and it will avoid i/o thread blocking while creating producer/consumer.

@rdhabalia rdhabalia added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Oct 31, 2016
@rdhabalia rdhabalia added this to the 1.16 milestone Oct 31, 2016
@rdhabalia rdhabalia self-assigned this Oct 31, 2016
@yahoocla
Copy link

CLA is valid!

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.

Looks good, just one blocking issue on the websocket

return;
if (service.isAuthorizationEnabled()) {
final String role = authRole;
isAuthorized(authRole).thenApply(isAuthorized -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be kind of different, because when the onWebSocketConnect() method completes we should have already done the authorization.

I think we need to block on the isAuhtorized() future, or find a way to have an async onWebSocketConnect()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.. WebSocketHandler.onWebSocketConnect() gets triggered after HttpConnection is established and as soon as HttpConnection is established : client also receives onConnect event.
So, HttpConnection.onCompleted() triggers WebSocketHandler.onWebSocketConnect().
So, by the time WebSocketHandler receives onWebSocketConnect() notification, client has already received onConnect() event.

  • However, after fixing websocket-test-case, I have verified that onWebSocketText(..) gets triggered at server only when server releases thread from onWebSocketConnect() So, reverting back to sync-zk behavior will make sure that producer will start receiving new messages on onWebSocketText() only after completion of authorization at onWebSocketConnect().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@merlimat addressed the change.

@rdhabalia rdhabalia force-pushed the async_auth branch 2 times, most recently from 4e29e6c to 1d8dab7 Compare November 1, 2016 17:41
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.

👍

@merlimat merlimat merged commit afaa63f into apache:master Nov 9, 2016
@rdhabalia rdhabalia deleted the async_auth branch January 23, 2017 22:09
sijie pushed a commit to sijie/pulsar that referenced this pull request Mar 4, 2018
hrsakai pushed a commit to hrsakai/pulsar that referenced this pull request Dec 10, 2020
* Implemented decompression on consumer side

* Fixed format
hangc0276 pushed a commit to hangc0276/pulsar that referenced this pull request May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants