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

Refactor and cleanup transport request handling #10730

Merged
merged 1 commit into from Apr 24, 2015

Conversation

kimchy
Copy link
Member

@kimchy kimchy commented Apr 22, 2015

This refactoring and cleanup is that each request handler ends up
implementing too many methods that can be provided when the request handler itself
is registered, including a prototype like class that can be used to instantiate
new request instances for streaming.

@kimchy kimchy force-pushed the transport_request_refactor branch from 53df1ed to 118f2e6 Compare April 22, 2015 16:08
kimchy added a commit to kimchy/elasticsearch that referenced this pull request Apr 22, 2015
This refactoring and cleanup is that each request handler ends up
implementing too many methods that can be provided when the request handler itself
is registered, including a prototype like class that can be used to instantiate
new request instances for streaming.
closes elastic#10730
@kimchy
Copy link
Member Author

kimchy commented Apr 22, 2015

since this is a big change, I would recommend focusing on TransportService change, almost all the rest are simply the implications of this change.

The other interesting ones are 3 base transport classes where an inner non static class was used (we can't have it, yay reflection), which I moved to the request class itself and used the notion of package internal shard id, this helps remove another class which I think simplifies the code anyhow.

@kimchy kimchy force-pushed the transport_request_refactor branch from 118f2e6 to 14614de Compare April 22, 2015 19:50
kimchy added a commit to kimchy/elasticsearch that referenced this pull request Apr 22, 2015
This refactoring and cleanup is that each request handler ends up
implementing too many methods that can be provided when the request handler itself
is registered, including a prototype like class that can be used to instantiate
new request instances for streaming.
closes elastic#10730
} catch (NoSuchMethodException e) {
throw new ElasticsearchIllegalStateException("failed to create constructor (does it have a default constructor?) for request " + request, e);
}
this.request.setAccessible(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should do this - the ctor must be public

Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok with this change if we open a blocker issue for fix this before 2.0

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to create an instance of the Request in here just to make sure we fail early.

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good, I will keep it like this for now, and we will fix it before 2.0

@s1monw
Copy link
Contributor

s1monw commented Apr 24, 2015

left a bunch of comments I love the cleanup and we should move quickly here

@kimchy kimchy force-pushed the transport_request_refactor branch from 6d58659 to 4b8407e Compare April 24, 2015 12:11
kimchy added a commit to kimchy/elasticsearch that referenced this pull request Apr 24, 2015
This refactoring and cleanup is that each request handler ends up
implementing too many methods that can be provided when the request handler itself
is registered, including a prototype like class that can be used to instantiate
new request instances for streaming.
closes elastic#10730
This refactoring and cleanup is that each request handler ends up
implementing too many methods that can be provided when the request handler itself
is registered, including a prototype like class that can be used to instantiate
new request instances for streaming.
closes elastic#10730
@kimchy kimchy force-pushed the transport_request_refactor branch from 4b8407e to 8dbb79c Compare April 24, 2015 12:12
@kimchy kimchy merged commit 8dbb79c into elastic:master Apr 24, 2015
@clintongormley clintongormley changed the title refactor and cleanup transport request handling Refactor and cleanup transport request handling Jun 7, 2015
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