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

Remove TransportService#registerRequestHandler leniency #20469

Merged
merged 7 commits into from
Sep 14, 2016

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Sep 14, 2016

TransportService#registerRequestHandler allowed to register
handlers more than once and issues an annoying warn log message when
this happens. This change simple throws an exception to prevent registering
the same handler more than once. This commit also removes the ability
to remove request handlers.

Relates to #20468

@javanna
Copy link
Member

javanna commented Sep 14, 2016

LGTM

@javanna
Copy link
Member

javanna commented Sep 14, 2016

Maybe as a followup we could add a small test that verifies this behaviour when we register a handler twice?

@s1monw
Copy link
Contributor Author

s1monw commented Sep 14, 2016

@javanna added a test

`TransportService#registerRequestHandler` allowed to register
handlers more than once and issues an annoying warn log message when
this happens. This change simple throws an exception to prevent regsitering
the same handler more than once. This commit also removes the ability
to remove request handlers.

Relates to elastic#20468
@s1monw s1monw force-pushed the remove_request_handler_leniency branch from f7b5f95 to e0c4c17 Compare September 14, 2016 10:28
@javanna
Copy link
Member

javanna commented Sep 14, 2016

thanks @s1monw still LGTM

requestHandlers = MapBuilder.newMapBuilder(requestHandlers).put(reg.getAction(), reg).immutableMap();
if (replaced != null) {
logger.warn("registered two transport handlers for action {}, handlers: {}, {}", reg.getAction(), reg, replaced);
if (requestHandlers.containsKey(reg.getAction())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

yay

@s1monw
Copy link
Contributor Author

s1monw commented Sep 14, 2016

@bleskes I update the PR again removing the hack in MockTransportService

@bleskes
Copy link
Contributor

bleskes commented Sep 14, 2016

LGTM

@s1monw
Copy link
Contributor Author

s1monw commented Sep 14, 2016

retest this please

@s1monw s1monw merged commit 17ddee7 into elastic:master Sep 14, 2016
s1monw added a commit that referenced this pull request Sep 14, 2016
`TransportService#registerRequestHandler` allowed to register
handlers more than once and issues an annoying warn log message when
this happens. This change simple throws an exception to prevent regsitering
the same handler more than once. This commit also removes the ability
to remove request handlers.

Relates to #20468
s1monw added a commit that referenced this pull request Sep 14, 2016
`TransportService#registerRequestHandler` allowed to register
handlers more than once and issues an annoying warn log message when
this happens. This change simple throws an exception to prevent regsitering
the same handler more than once. This commit also removes the ability
to remove request handlers.

Relates to #20468
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants