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

Upgrade eureka version #3257

Merged
merged 2 commits into from Nov 7, 2018
Merged

Upgrade eureka version #3257

merged 2 commits into from Nov 7, 2018

Conversation

holy12345
Copy link
Contributor

@holy12345 holy12345 commented Oct 29, 2018

Fixes #3255

@ryanjbaxter
Copy link
Contributor

The build breaks with this change

@holy12345
Copy link
Contributor Author

HI @ryanjbaxter

I see it and I will fix it. thanks

@codecov-io
Copy link

codecov-io commented Nov 1, 2018

Codecov Report

Merging #3257 into master will increase coverage by 0.32%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3257      +/-   ##
============================================
+ Coverage     65.46%   65.78%   +0.32%     
- Complexity     1475     1480       +5     
============================================
  Files           188      188              
  Lines          6943     6965      +22     
  Branches        846      847       +1     
============================================
+ Hits           4545     4582      +37     
+ Misses         2085     2069      -16     
- Partials        313      314       +1
Impacted Files Coverage Δ Complexity Δ
...x/eureka/server/EurekaServerAutoConfiguration.java 91.89% <100%> (ø) 16 <0> (ø) ⬇️
.../cloud/netflix/eureka/server/InstanceRegistry.java 81.25% <100%> (ø) 16 <0> (ø) ⬇️
.../netflix/eureka/server/EurekaServerConfigBean.java 22.04% <25%> (+0.26%) 33 <3> (+3) ⬆️
...flix/eureka/server/InstanceRegistryProperties.java 55.55% <50%> (ø) 3 <1> (ø) ⬇️
...x/ribbon/apache/HttpClientRibbonConfiguration.java 92.68% <0%> (-4.88%) 3% <0%> (ø)
...oud/netflix/zuul/filters/post/SendErrorFilter.java 72.72% <0%> (-3.64%) 16% <0%> (-1%)
...k/cloud/netflix/eureka/EurekaClientConfigBean.java 30.1% <0%> (+0.66%) 47% <0%> (+2%) ⬆️
...cloud/netflix/eureka/EurekaInstanceConfigBean.java 69.03% <0%> (+16.75%) 54% <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 6859b7a...e2e0aef. Read the comment docs.

@holy12345
Copy link
Contributor Author

Hi @ryanjbaxter
I update the code. could you please help me review the code?
thanks

Copy link
Contributor

@ryanjbaxter ryanjbaxter left a comment

Choose a reason for hiding this comment

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

Question regarding the changes

@holy12345
Copy link
Contributor Author

Hi @ryanjbaxter thanks help me review :)

Let me explain details
EurekaServerConfigBean.java I add four private properties because EurekaServerConfig has add those.

Below in 1.9.4 eureka version :
initialCapacityOfResponseCache Associated PR: Netflix/eureka#1096

useAwsAsgApi Associated PR: Netflix/eureka#1108

expectedClientRenewalIntervalSeconds
Associated PR:Netflix/eureka#1093

myUrl Associated PR:Netflix/eureka#1107

On the other hand Eureka server use expectedNumberOfClientsSendingRenews replace expectedNumberOfRenewsPerMin. this is the reason why I change the InstanceRegistry.java and InstanceRegistryProperties.java and this PR you can see Netflix/eureka#1093

Copy link
Contributor

@ryanjbaxter ryanjbaxter left a comment

Choose a reason for hiding this comment

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

Thanks I just wanted to make sure those default values were actually coming from somewhere and not being made up.

@ryanjbaxter ryanjbaxter added this to In progress in Greenwich.M2 via automation Nov 2, 2018
@ryanjbaxter ryanjbaxter added this to the 2.1.0.RC1 milestone Nov 2, 2018
* server can adjust its eviction policy to the number of registrations (when it's
* zero, even a successful registration won't reset the rate threshold in
* InstanceRegistry.register()).
*/
@Value("${eureka.server.expectedNumberOfRenewsPerMin:1}") // for backwards compatibility
private int expectedNumberOfRenewsPerMin = 1;
@Value("${eureka.server.expectedNumberOfClientsSendingRenews:1}") // for backwards compatibility
Copy link
Contributor

Choose a reason for hiding this comment

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

@spencergibb do you think we should support the old property as well as new property to avoid breaking people that transition to Greenwich?

Copy link

Choose a reason for hiding this comment

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

That would be nice since it does break others, like this: https://github.com/twinformatics/eureka-consul-adapter

Copy link
Contributor

Choose a reason for hiding this comment

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

@maximusfloydus could you open a separate issue?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Haha, I opened the issue and fixed it.

@holy12345
Copy link
Contributor Author

Any update? thanks

@ryanjbaxter ryanjbaxter merged commit 195cd3e into spring-cloud:master Nov 7, 2018
Greenwich.M2 automation moved this from In progress to Done Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Greenwich.M2
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants