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

fixes for #649 #652

Merged
merged 3 commits into from
Feb 26, 2020
Merged

fixes for #649 #652

merged 3 commits into from
Feb 26, 2020

Conversation

stevehu
Copy link
Contributor

@stevehu stevehu commented Feb 25, 2020

No description provided.

@codecov-io
Copy link

codecov-io commented Feb 25, 2020

Codecov Report

Merging #652 into 1.6.x will decrease coverage by <.01%.
The diff coverage is 20%.

Impacted file tree graph

@@             Coverage Diff              @@
##              1.6.x     #652      +/-   ##
============================================
- Coverage     49.92%   49.91%   -0.01%     
  Complexity     1914     1914              
============================================
  Files           261      261              
  Lines         11688    11690       +2     
  Branches       1650     1651       +1     
============================================
  Hits           5835     5835              
- Misses         5166     5167       +1     
- Partials        687      688       +1
Impacted Files Coverage Δ Complexity Δ
...a/com/networknt/consul/ConsulHeartbeatManager.java 81.53% <ø> (ø) 11 <0> (ø) ⬇️
.../com/networknt/consul/client/ConsulClientImpl.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../main/java/com/networknt/consul/ConsulService.java 84% <33.33%> (ø) 13 <0> (ø) ⬇️
...n/java/com/networknt/client/oauth/OauthHelper.java 36.93% <50%> (-0.12%) 28 <0> (ø)

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 96a5096...f14e9e2. Read the comment docs.

@stevehu
Copy link
Contributor Author

stevehu commented Feb 25, 2020

@ChenYan71 I have created the PR from your branch and merge the latest code to your branch. While reviewing the changes, I found that we cannot make the change in the ConsulClientImpl to use HTTP/2 connection when the https protocol or enableHttp2 is true. In case that enableHttp2 is not in the config file, even we use the Http2Connect but the request we send to the Consul will still be HTTP/1.1 which cannot leverage the multiplexing. What will happen if we rollback this change and only leave the ConsulRegistry change to check if the urls is empty? Thanks.

@stevehu
Copy link
Contributor Author

stevehu commented Feb 26, 2020

The isHttp2 is tested and confirmed by @ChenYan71 and it is ready to merge.

@stevehu stevehu merged commit a2061c3 into 1.6.x Feb 26, 2020
@stevehu stevehu deleted the issue_649 branch February 26, 2020 15:56
stevehu added a commit that referenced this pull request Feb 26, 2020
*  fixes for #649

* fixes #649 move the http2 check into a private method

Co-authored-by: Gavin (Yan) Chen <gavinyan.han@gmail.com>
younggwon1 pushed a commit to younggwon1/light-4j that referenced this pull request Feb 10, 2024
*  fixes for networknt#649

* fixes networknt#649 move the http2 check into a private method

Co-authored-by: Gavin (Yan) Chen <gavinyan.han@gmail.com>
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.

3 participants