Skip to content

feat: Adds RestTemplate default ConnectTimeout / ConnectRequestTimeout - #4148

Merged
OlgaMaciaszek merged 4 commits into
spring-cloud:3.1.xfrom
kworkbee:3.1.x
Dec 13, 2022
Merged

feat: Adds RestTemplate default ConnectTimeout / ConnectRequestTimeout#4148
OlgaMaciaszek merged 4 commits into
spring-cloud:3.1.xfrom
kworkbee:3.1.x

Conversation

@kworkbee

@kworkbee kworkbee commented Dec 8, 2022

Copy link
Copy Markdown
Contributor

Fixes gh-4110

@codecov

codecov Bot commented Dec 8, 2022

Copy link
Copy Markdown

Codecov Report

Base: 55.61% // Head: 55.52% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (0a848d9) compared to base (b467a2f).
Patch coverage: 54.54% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##              3.1.x    #4148      +/-   ##
============================================
- Coverage     55.61%   55.52%   -0.09%     
- Complexity      529      534       +5     
============================================
  Files            58       59       +1     
  Lines          2503     2534      +31     
  Branches        236      239       +3     
============================================
+ Hits           1392     1407      +15     
- Misses         1023     1038      +15     
- Partials         88       89       +1     
Impacted Files Coverage Δ
.../netflix/eureka/RestTemplateTimeoutProperties.java 33.33% <33.33%> (ø)
...DefaultEurekaClientHttpRequestFactorySupplier.java 75.00% <90.90%> (+15.00%) ⬆️
...nfig/DiscoveryClientOptionalArgsConfiguration.java 86.95% <100.00%> (ø)
.../netflix/eureka/server/EurekaServerConfigBean.java 22.50% <0.00%> (-0.24%) ⬇️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kworkbee

kworkbee commented Dec 9, 2022

Copy link
Copy Markdown
Contributor Author

Wrote related code to explicitly declare custom timeouts in eureka.client.rest-template-timeout.

@OlgaMaciaszek OlgaMaciaszek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@kworkbee Thanks a lot for submitting this PR. In general, it looks good, but there are a few minor issues. I've added comments - please address them. Also, since this contains breaking changes (method signature and constructor changes), we'd need to move them to 2022.x only (submitting against main branch - we're realeasing GA on the 14th, so it'd need to be done soon). If you want this to be made available also for the 2021.x release train (3.1.x branch), that's fine by me, but you'll need to make it backwards compatible (i.e., just adding new constructors and method signatures while deprecating the old ones, withou editing or removing existing public members).

@kworkbee

This comment was marked as off-topic.

- eureka.client.rest-template-timeout

Fixes spring-cloudgh-4110
@kworkbee

Copy link
Copy Markdown
Contributor Author

@OlgaMaciaszek Thank you for your code review. In addition to the feedback you gave me, I rewritten the code to keep the code cleaner, so please review it.

If possible, I hope it will be reflected in the upcoming release.
Once again, thank you very much for your help.

P.S. This is my first time to contribute, so I'm very nervous but so much excited!

@kworkbee

Copy link
Copy Markdown
Contributor Author

Because Configuration Properties declaration moved, I removed the test in the existing test class. Is it a big problem for the 'codecov/project' error to occur as you can see?

@OlgaMaciaszek OlgaMaciaszek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@kworkbee thanks for your contribution; its' a good enhancement :) . Please address the comments I've added for the new implementation.

@OlgaMaciaszek

Copy link
Copy Markdown
Collaborator

Because Configuration Properties declaration moved, I removed the test in the existing test class. Is it a big problem for the 'codecov/project' error to occur as you can see?

No, the delta threshold is set too high. We should change it in future. I don't think this change requires additional tests.

@kworkbee

Copy link
Copy Markdown
Contributor Author

Again, modifies code.
Hope I correctly understand your intentions.
(Having poor English and code writing, asks for warm encouragement from junior who just started own career. (:shy))

@OlgaMaciaszek OlgaMaciaszek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @kworkbee, this is getting close to merge now. There's one more minor change left in the code - please see the comment. Also, please document the new functionality in spring-cloud-netflix.adoc (There's a phrase that sais: "If you want to customize the RestTemplate used by the Eureka HTTP Client you may want to create a bean of EurekaClientHttpRequestFactorySupplier and provide your own logic for generating a ClientHttpRequestFactory instance." - you can document your feature directly after that).

@OlgaMaciaszek OlgaMaciaszek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, @kworkbee. LGTM.

@OlgaMaciaszek
OlgaMaciaszek merged commit 5844550 into spring-cloud:3.1.x Dec 13, 2022
@kworkbee

Copy link
Copy Markdown
Contributor Author

Thank you. It was a good experience.

@OlgaMaciaszek

Copy link
Copy Markdown
Collaborator

Thanks for contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Getting all instance registry info without timeout

3 participants