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 ability to plug-in TransportService #20505

Merged
merged 7 commits into from Sep 16, 2016

Conversation

Projects
None yet
3 participants
@s1monw
Copy link
Contributor

commented Sep 15, 2016

TransportService is such a central part of the core server, replacing
it's implementation is risky and can cause serious issues. This change removes the ability to
plug in TransportService but allows registering a TransportInterceptor that enables
plugins to intercept requests on both the sender and the receiver ends. This is a commonly used
and overwritten functionality but encapsulates the custom code in a contained manner.

Remove ability to plug-in NetworkService
NetworkService is such a central part of the core server, replacing
it's implemenation is risky and can cause serious issues. This change removes the ability to
plug in NetworkService but allows regsitering a `TransportInterceptor` that enables
plugins to intercept requests on both the sender and the receiver ends. This is a commonly used
and overwritten functionality but encapsulates the custom code in a contained manner.

@s1monw s1monw changed the title Remove ability to plug-in NetworkService Remove ability to plug-in TransportService Sep 15, 2016

@rjernst
Copy link
Member

left a comment

LGTM. Perhaps we could/should move TransportInterceptor out from an inner class? My thought is just that plugins knowing about the TransportService is not necessary (TransportService simply uses the new interface).

/**
* Registers a new {@link org.elasticsearch.transport.TransportService.TransportInterceptor}
*/
public void addTransportInterceptor(TransportService.TransportInterceptor interceptor) {

This comment has been minimized.

Copy link
@rjernst

rjernst Sep 15, 2016

Member

Is this just temporary, and can/should be replaced with a pull based interface in a followup?

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 15, 2016

Author Contributor

yeah I didn't wanna do that at the same time

* {@link #sendRequest(DiscoveryNode, String, TransportRequest, TransportRequestOptions, TransportResponseHandler)}.
* This allows plugins to perform actions on each send request including modifying the request context etc.
*/
default AsyncSender asyncSender(AsyncSender sender) {

This comment has been minimized.

Copy link
@rjernst

rjernst Sep 15, 2016

Member

nit: can this be named in parallel, ie interceptSender?

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 15, 2016

Author Contributor

sure

s1monw added some commits Sep 15, 2016

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2016

@rjernst I applied your comments

@rjernst

This comment has been minimized.

Copy link
Member

commented Sep 15, 2016

Still LGTM

@s1monw s1monw merged commit f5daa16 into elastic:master Sep 16, 2016

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

s1monw added a commit that referenced this pull request Sep 16, 2016

Remove ability to plug-in TransportService (#20505)
TransportService is such a central part of the core server, replacing
it's implementation is risky and can cause serious issues. This change removes the ability to
plug in TransportService but allows registering a TransportInterceptor that enables
plugins to intercept requests on both the sender and the receiver ends. This is a commonly used
and overwritten functionality but encapsulates the custom code in a contained manner.

s1monw added a commit that referenced this pull request Sep 16, 2016

Remove ability to plug-in TransportService (#20505)
TransportService is such a central part of the core server, replacing
it's implementation is risky and can cause serious issues. This change removes the ability to
plug in TransportService but allows registering a TransportInterceptor that enables
plugins to intercept requests on both the sender and the receiver ends. This is a commonly used
and overwritten functionality but encapsulates the custom code in a contained manner.

@clintongormley clintongormley removed the v5.1.1 label Dec 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.