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

Transport service #170

Closed
wants to merge 10 commits into from
Closed

Transport service #170

wants to merge 10 commits into from

Conversation

mloufra
Copy link
Contributor

@mloufra mloufra commented Oct 7, 2022

Description

To avoid passing the object of extensionRunner in ExtensionsRunner main function, move all of the required methods and variables to NettyTransport file
Address comment from previous PR.

Issues Resolved

#166
#116

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@mloufra mloufra requested review from dbwiddis and removed request for dbwiddis October 7, 2022 16:36
@mloufra mloufra marked this pull request as ready for review October 7, 2022 19:36
@mloufra mloufra requested review from a team and dbwiddis October 7, 2022 19:36
@codecov-commenter
Copy link

Codecov Report

Merging #170 (42f5281) into main (8fbcc6e) will increase coverage by 0.43%.
The diff coverage is 80.00%.

@@             Coverage Diff              @@
##               main     #170      +/-   ##
============================================
+ Coverage     66.00%   66.44%   +0.43%     
  Complexity       96       96              
============================================
  Files            25       25              
  Lines           456      456              
  Branches         13       12       -1     
============================================
+ Hits            301      303       +2     
+ Misses          147      146       -1     
+ Partials          8        7       -1     
Impacted Files Coverage Δ
...main/java/org/opensearch/sdk/ExtensionsRunner.java 63.23% <50.00%> (ø)
...c/main/java/org/opensearch/sdk/NettyTransport.java 100.00% <100.00%> (+7.14%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

Code changes look good. Just need some doc fixes on the constructor javadoc.

src/main/java/org/opensearch/sdk/ExtensionsRunner.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/sdk/ExtensionsRunner.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/sdk/NettyTransport.java Outdated Show resolved Hide resolved
src/test/java/org/opensearch/sdk/TestNetty4Transport.java Outdated Show resolved Hide resolved
@mloufra mloufra marked this pull request as draft October 11, 2022 18:46
@dbwiddis dbwiddis force-pushed the main branch 3 times, most recently from f2399fb to 6eab1a5 Compare October 12, 2022 01:09
@dbwiddis dbwiddis marked this pull request as ready for review October 12, 2022 03:06
dbwiddis
dbwiddis previously approved these changes Oct 12, 2022
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

Fixed the bind errors by moving the netty initialization out of the constructor in the Extensions Runner and backed out the "hack" solution from intervening PR. The rest is what was ready to go before this weekend and is GTG now! :)

@dbwiddis dbwiddis requested a review from owaiskazi19 October 12, 2022 03:19
@dbwiddis
Copy link
Member

@ryanbogan can you help resolve the conflicts of this PR with #146 which was recently merged? Build is failing on the additional arguments to new NettyTransport(runner).initializeExtensionTransportService(runner.getSettings());

@mloufra mloufra closed this by deleting the head repository Oct 12, 2022
@mloufra mloufra reopened this Oct 12, 2022
@ryanbogan
Copy link
Member

ryanbogan commented Oct 12, 2022

That method should take a threadPool parameter too. Adding that will let the compile check pass

@ryanbogan
Copy link
Member

I can't access the branch with the changes for some reason. The only branch I see on @mloufra 's fork is main.

@mloufra
Copy link
Contributor Author

mloufra commented Oct 12, 2022

I can't access the branch with the changes for some reason. The only branch I see on @mloufra 's fork is main.

I fork our sdk repo again this morning. That's why only main branch in my fork repo.

@owaiskazi19
Copy link
Member

owaiskazi19 commented Oct 12, 2022

I fork our sdk repo again this morning. That's why only main branch in my fork repo.

@mloufra you should close this PR and raise again with the new fork repo as you won't be able to access transportService branch anymore on your fork.

@mloufra
Copy link
Contributor Author

mloufra commented Oct 12, 2022

I fork our sdk repo again this morning. That's why only main branch in my fork repo.

@mloufra you should close this PR and raise again with the new fork repo as you won't be able to access transportService branch anymore on your fork.

Thanks.

@mloufra mloufra closed this Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants