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

Integration with REST based Light Config Server #612

Merged
merged 5 commits into from Nov 23, 2019

Conversation

santoshaherkar
Copy link
Member

No description provided.

@codecov-io
Copy link

codecov-io commented Sep 23, 2019

Codecov Report

Merging #612 into 1.6.x will decrease coverage by 0.42%.
The diff coverage is 4.26%.

Impacted file tree graph

@@             Coverage Diff              @@
##              1.6.x     #612      +/-   ##
============================================
- Coverage     51.15%   50.72%   -0.43%     
- Complexity     1905     1912       +7     
============================================
  Files           258      258              
  Lines         11386    11489     +103     
  Branches       1615     1621       +6     
============================================
+ Hits           5824     5828       +4     
- Misses         4880     4975      +95     
- Partials        682      686       +4
Impacted Files Coverage Δ Complexity Δ
...java/com/networknt/server/DefaultConfigLoader.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...c/main/java/com/networknt/server/ServerConfig.java 70.21% <14.28%> (-4.79%) 40 <0> (ø)
...rc/main/java/com/networknt/client/Http2Client.java 44.93% <31.57%> (-1.05%) 42 <12> (+7)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09c6736...0aaa3a1. Read the comment docs.

@stevehu
Copy link
Contributor

stevehu commented Oct 2, 2019

Is there any particular reason that the apache httpclient is introduced? Why cannot we use Http2Client from the client module?

@santoshaherkar
Copy link
Member Author

@stevehu we used apache client to connect to config server initially with its own ssl context & we wanted to keep this ssl context separated from light4j context (for this initial config server connection). Once the connection is made with config server, it downloads the configs/certs and make them available for Light4j ssl context. If we use HTTP2Client upfront for this purpose, then it would setup ssl context during config server connection only and then it would be difficult to revert it (as those properties are static).
Your suggestions are welcome here.

@stevehu
Copy link
Contributor

stevehu commented Oct 2, 2019

@santoshaherkar This is a very complicated bootstrap issue and I never thought about using another library to solve the problem before. Let's have an offline discussion on this issue. Thanks.

@miklish
Copy link
Collaborator

miklish commented Oct 2, 2019

@stevehu Do you still have concerns with the Apache client? I know @santoshaherkar @jsu216 and @zabooma spent a long time working on this problem before arriving at this solution. It is well vetted.

@stevehu
Copy link
Contributor

stevehu commented Oct 21, 2019

It seems very easy to use another XnioSsl object to make a connection with Http2Client. I have tested with a live server running locally from the DefaultConfigLoaderIT class and the Health endpoint returns OK. Please review and let us know if you have any questions or concerns. Thanks.

@jsu216
Copy link
Contributor

jsu216 commented Oct 22, 2019

@stevehu, I tried using new XionSsl object to create a connection with Http2Client, but surprisingly it had same chances of exceptions (if not more) thrown when close the connection. The test I did was for the issue #616. The cexceptions thrown seem to be more like a debug message than errors.

@stevehu
Copy link
Contributor

stevehu commented Oct 22, 2019

@jsu216 I know there are a lot of debug exceptions thrown in Undertow and I think they should change the level to trace. These exceptions are caused by the HTTP/2 server not following the spec gracefully and we can simply ignore it. If the logging level is Error, then it is a different story.

@jsu216
Copy link
Contributor

jsu216 commented Oct 22, 2019

The ERROR level messages happened in #616 are either Undertow/Xnio bugs or should be changed to DEBUG level. Because there is nothing wrong in our code and there seems to be no impact to the functionality.

@stevehu
Copy link
Contributor

stevehu commented Oct 22, 2019

@jsu216 I totally agree with you and it seems this error only happens with the Influxdb connection which is implemented in Golang. Consul is implemented in Golang but I have never seen this error with Consul connections. There might be something in the Influxdb that causes the error. Do you think we should open a ticket in the Undertow repo? If yes, we need to find a way to reliably reproduce it.

@jsu216
Copy link
Contributor

jsu216 commented Oct 22, 2019

@stevehu The errors happen not only to influxdb connections. My test was to connect to another light4j service. There are open tickets in undertow/redhat. I post the links in the my comments of the issue #616 .
Errors were populated into log after trying to close the connection. The pattern is that you create a connection, use it and close it, repeat this every a few seconds. Most of the time you will get errors (NPE) within 20 mins. No exception was thrown in that case, which makes me think it is either a bug or debug message only.

@stevehu
Copy link
Contributor

stevehu commented Oct 22, 2019

@jsu216 Sorry. I missed the links to Undertow and RedHat issues. The issue was opened for 2.0.8 and I am wondering if we should upgrade to the latest version to see if it is already resolved. Although the chances are very slim :)

@stevehu stevehu merged commit dac3564 into 1.6.x Nov 23, 2019
@stevehu stevehu deleted the feat/integ-with-rest-config-server branch November 23, 2019 17:23
@stevehu
Copy link
Contributor

stevehu commented Nov 23, 2019

stevehu pushed a commit that referenced this pull request Nov 23, 2019
stevehu added a commit that referenced this pull request Nov 23, 2019
younggwon1 pushed a commit to younggwon1/light-4j that referenced this pull request Feb 10, 2024
younggwon1 pushed a commit to younggwon1/light-4j that referenced this pull request Feb 10, 2024
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.

None yet

5 participants