-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Conversation
53df1ed
to
118f2e6
Compare
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
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. |
118f2e6
to
14614de
Compare
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a bunch of comments I love the cleanup and we should move quickly here |
6d58659
to
4b8407e
Compare
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
4b8407e
to
8dbb79c
Compare
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.