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

Implement socket and server ChannelContexts #28275

Merged
merged 5 commits into from
Jan 18, 2018

Conversation

Tim-Brooks
Copy link
Contributor

This commit is related to #27260. Currently have a channel context that
implements reading and writing logic for socket channels. Additionally,
we have exception contexts to handle exceptions. And accepting contexts
to handle accepted channels. This PR introduces a ChannelContext that
handles close and exception handling for all channel types.
Additionally, it has implementers that provide specific functionality
for socket channels (read and writing). And specific functionality for
server channels (accepting).

@Tim-Brooks Tim-Brooks added >non-issue review :Distributed/Network Http and internode communication implementations v7.0.0 labels Jan 18, 2018
Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

I really just left one question

private ChannelContext context;
private BiConsumer<NioSocketChannel, Exception> exceptionContext;
private final AtomicBoolean contextSet = new AtomicBoolean(false);
private SocketChannelContext context;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we have a context here again? the superclass has it aready? Can't we use a getter that has the right type or a generic?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any change we can untangle the cyclic dependencies here and pass the context in as a ctor argument? or can we pass it as method arguments instead and don't have it as a member?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the duplicate contexts. Now each stances (NioServerSocketChannel and NioSocketChannel) has their own. I could store it in the super class parameterized. But AbstractNioChannel is already parameterized once and it would get pretty big.

Probably not possible to change the cyclic relationship right now. Context needs the channel. So it cannot be a ctor argument without allowing this to escape. Channel needs context as a field, as it is the hook into lower level selector operations. But going forward I have some ideas about how to continue to reduce this relationship.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool lets move on with this.

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

LGTM

@Tim-Brooks Tim-Brooks merged commit a6a57a7 into elastic:master Jan 18, 2018
martijnvg added a commit that referenced this pull request Jan 22, 2018
* es/master: (38 commits)
  Build: Add pom generation to meta plugins (#28321)
  Add 6.3 version constant to master
  Minor improvements to translog docs (#28237)
  [Docs] Remove typo in painless-getting-started.asciidoc
  Build: Fix meta plugin usage in integ test clusters (#28307)
  Painless: Add spi jar that will be published for extending whitelists (#28302)
  mistyping in one of the highlighting examples comment -> content (#28139)
  Documents applicability of term query to range type (#28166)
  Build: Omit dependency licenses check for elasticsearch deps (#28304)
  Clean up commits when global checkpoint advanced (#28140)
  Implement socket and server ChannelContexts (#28275)
  Plugins: Fix meta plugins to install bundled plugins with their real name (#28285)
  Build: Fix meta plugin integ test installation (#28286)
  Modify Abstract transport tests to use impls (#28270)
  Fork Groovy compiler onto compile Java home
  [Docs] Update tophits-aggregation.asciidoc (#28273)
  Docs: match between snippet to its description (#28296)
  [TEST] fix RequestTests#testSearch in case search source is not set
  REST high-level client: remove index suffix from indices client method names (#28263)
  Fix simple_query_string on invalid input (#28219)
  ...
@Tim-Brooks Tim-Brooks deleted the next_step2 branch December 10, 2018 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Network Http and internode communication implementations >non-issue v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants