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

Re-create connection manager on "zuul.host.*" property change #3407

Merged
merged 2 commits into from Sep 3, 2019

Conversation

denysivano
Copy link
Contributor

When changing zuul.host.* properties, it makes a sense to re-create HttpClientConnectionManager along with HttpClient to make maxTotalConnections and maxConnectionsPerRoute properties dynamically configurable.

It also prevents the use of shut down connection pool (fixes gh-3406).

@denysivano denysivano marked this pull request as ready for review March 2, 2019 17:12
@@ -137,6 +138,9 @@ public void onPropertyChange(EnvironmentChangeEvent event) {
catch (IOException ex) {
log.error("error closing client", ex);
}
// Re-create connection manager (may be shut down on HTTP client close)
SimpleHostRoutingFilter.this.connectionManager.shutdown();
Copy link
Member

Choose a reason for hiding this comment

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

This should move in the above try catch or in its own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

connectionManager.shutdown() doesn't throw IOException, but httpClient.close() does.

Is there need to catch RuntimeException for connectionManager.shutdown() ?

Copy link
Member

Choose a reason for hiding this comment

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

you're correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapped it in e4f3f60

@codecov-io
Copy link

Codecov Report

Merging #3407 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3407      +/-   ##
============================================
+ Coverage     65.42%   65.42%   +<.01%     
- Complexity     1516     1517       +1     
============================================
  Files           189      189              
  Lines          7247     7249       +2     
  Branches        863      862       -1     
============================================
+ Hits           4741     4743       +2     
+ Misses         2189     2188       -1     
- Partials        317      318       +1
Impacted Files Coverage Δ Complexity Δ
...ix/zuul/filters/route/SimpleHostRoutingFilter.java 76.11% <100%> (+1.24%) 44 <2> (+2) ⬆️
...oud/netflix/zuul/filters/post/SendErrorFilter.java 74.57% <0%> (-3.39%) 16% <0%> (-1%)

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 435e94c...e022051. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Mar 2, 2019

Codecov Report

Merging #3407 into master will increase coverage by 0.05%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3407      +/-   ##
============================================
+ Coverage     65.42%   65.47%   +0.05%     
- Complexity     1516     1518       +2     
============================================
  Files           189      189              
  Lines          7247     7252       +5     
  Branches        863      862       -1     
============================================
+ Hits           4741     4748       +7     
+ Misses         2189     2187       -2     
  Partials        317      317
Impacted Files Coverage Δ Complexity Δ
...ix/zuul/filters/route/SimpleHostRoutingFilter.java 75.49% <83.33%> (+0.61%) 44 <2> (+2) ⬆️
...oud/netflix/zuul/filters/post/SendErrorFilter.java 74.57% <0%> (-3.39%) 16% <0%> (-1%)
...x/ribbon/apache/HttpClientRibbonConfiguration.java 97.56% <0%> (+4.87%) 3% <0%> (ø) ⬇️
...etflix/turbine/stream/HystrixStreamAggregator.java 78.94% <0%> (+5.26%) 9% <0%> (+1%) ⬆️

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 435e94c...e4f3f60. Read the comment docs.

@spencergibb spencergibb removed this from In progress in Greenwich.SR2 Jul 3, 2019
@spencergibb spencergibb added this to In progress in Greenwich.SR3 via automation Jul 3, 2019
@spencergibb spencergibb added this to In progress in Hoxton.M3 via automation Aug 22, 2019
@spencergibb spencergibb added this to the 2.1.3.RELEASE milestone Aug 22, 2019
@ryanjbaxter ryanjbaxter merged commit 0ea0472 into spring-cloud:master Sep 3, 2019
Hoxton.M3 automation moved this from In progress to Done Sep 3, 2019
Greenwich.SR3 automation moved this from In progress to Done Sep 3, 2019
@denysivano
Copy link
Contributor Author

@ryanjbaxter Is there any plans to backport this PR to 2.1.x branch? As I can see from the milestone, this fix had to be included in 2.1.3.RELEASE (Greenwich.SR3), but it wasn't.

ryanjbaxter added a commit that referenced this pull request Oct 1, 2019
@ryanjbaxter
Copy link
Contributor

@denysivano sorry about that I applied it to the 2.1.x branch it will be in Greenwich.SR4

@ryanjbaxter ryanjbaxter added this to In progress in Greenwich.SR4 via automation Oct 1, 2019
@ryanjbaxter ryanjbaxter moved this from In progress to Done in Greenwich.SR4 Nov 14, 2019
sabareeshkkanan added a commit to sabareeshkkanan/spring-cloud-netflix that referenced this pull request Dec 20, 2019
* Added symbolic link of index.adoc

* Added symbolic link of index.adoc

* set replication client filters in RefreshablePeerEurekaNodes (spring-cloud#3610)

fixes spring-cloudgh-3554

* Update SNAPSHOT to 2.2.0.M2

* Going back to snapshots

* Adding spring cloud circuitbreaker hystrix implementation

* Separated circuitbreaker auto config to its own file

* Initialize remoteRegionAppWhitelist with default value (spring-cloud#3634)

* Moves non-netflix dependencies out of bom into root pom.

fixes spring-cloudgh-3639

* Removes dependency management for okhttp3

* Moves okhttp3 dep mgmt back to bom

* Re-create connection manager on "zuul.host.*" property change (spring-cloud#3407)

* Re-create connection manager on "zuul.host.*" property change 

Fixes spring-cloud#3406

* Wrap connectionManager.shutdown() in try/catch block. Polishing

* Removes okhttp3 again after added back in older branch.

* Updating readme with note about building spring-cloud-netflix-hystrix-contract.  Fixes spring-cloud#3497

* Fromatting

* Upgrades eureka to 1.9.13 and excludes compactmap.

fixes spring-cloudgh-3636

* Update SNAPSHOT to 2.1.3.RELEASE

* Going back to snapshots

* Bumping versions to 2.1.4.BUILD-SNAPSHOT after release

* Add Spring Cloud LoadBalancer starter to Eureka starters. Fixes spring-cloudgh-3646. (spring-cloud#3647)

* Update docs. (spring-cloud#3650)

* ConditionalOnMissingBean on formBodyWrapperFilter, debugFilter… (spring-cloud#3609)

* Bumping versions

* Remove spring.provides.

* removes useless comments

* Fix RestTemplateEurekaHttpClient status update endpoint.  Fixes spring-cloud#3571 (spring-cloud#3657)

* polish

* Updates health check handler to use new StatusAggregator

* Gh 3409 turbine stream test (spring-cloud#3665)

* Add stream-test-support.

* Fix condition.

* Remove outdated workaround.

* Bumping versions

* Optimize code of eureka (spring-cloud#3660)

* Optimize code of eureka

* Merge newest code

* fix checkstyle bug

* Gh 3464 upgrade scc new (spring-cloud#3667)

* Upgrade Spring Cloud Contract version to 2.1.3.RELEASE.

* Gh 3409 turbine stream test (spring-cloud#3665)

* Add stream-test-support.

* Fix condition.

* Remove outdated workaround.

(cherry picked from commit 24f5e0b)

* Updates to use "components" rather than "details"

See spring-projects/spring-boot#17929

* Applying spring-cloud#3407 to the 2.1.x branch

* Added support for reactive service discovery

* Update SNAPSHOT to 2.2.0.M3

* Going back to snapshots

* Update SNAPSHOT to 2.2.0.RC1

* Going back to snapshots

* removing resource class from circle config

* Add property to disable spring cloud circuit breaker for hystrix

* Fix command key configuration.  Use id as command key and class as group key.

* Adding configuration metadata for spring cloud circuitbreaker

* Bumping versions

* Fix some dependencies that show up in the wrong scope

Apparently you can build on the command line but Eclipse is fussy
now and wouldn't compile these projects without explicit
dependencies.

* Also add build helper config for contract tests

* Added maven flatten plugin

* Bumping versions

* Update SNAPSHOT to 2.2.0.RC2

* Going back to snapshots

* Update configuration to use proxyBeanMethods=false.  Fixes spring-cloud#3677

* Create security.md

* Update issue templates

* Bumping versions

* Bumping versions

* Update SNAPSHOT to 2.2.0.RELEASE

* Going back to snapshots

* Bumping versions to 2.2.1.BUILD-SNAPSHOT after release

* Bumping versions

* Fix typo: clas -> class (spring-cloud#3710)

* removes .flattened-pom.xml

* ignores .flattened-pom.xml

* Gh 3718 add zoned loadbalancer instrumentation (spring-cloud#3720)

* Add instrumentation for zoned LoadBalancer.

* Add documentation.

* Fix after review.

* Fix after review.

* Update SNAPSHOT to 2.2.1.RELEASE

* Going back to snapshots

* Bumping versions to 2.2.2.BUILD-SNAPSHOT after release

Co-authored-by: Marcin Grzejszczak <marcin@grzejszczak.pl>
Co-authored-by: Yuxin Bai <LittleBaiBai@users.noreply.github.com>
Co-authored-by: Spencer Gibb <spencer@gibb.tech>
Co-authored-by: Spring Buildmaster <buildmaster@springframework.org>
Co-authored-by: Ryan Baxter <rbaxter@pivotal.io>
Co-authored-by: emilnkrastev <emilnkrastev@gmail.com>
Co-authored-by: Denys Ivano <denys.ivano@gmail.com>
Co-authored-by: Olga Maciaszek-Sharma <olga.maciaszek@gmail.com>
Co-authored-by: Rafał Żukowski <rzukow@gmail.com>
Co-authored-by: OLPMO <OLPMO@users.noreply.github.com>
Co-authored-by: Tim Ysewyn <Tim.Ysewyn@me.com>
Co-authored-by: Dave Syer <david_syer@hotmail.com>
Co-authored-by: Deepika Mohan <deepikadevidm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Greenwich.SR3
  
Done
Greenwich.SR4
  
Done
Hoxton.M3
  
Done
Development

Successfully merging this pull request may close these issues.

Connection pool shut down on "zuul.host.*" property change
5 participants