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.

Sign up for GitHub

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

Jump to bottom

Use milliseconds as an interval unit in EurekaEndpointGroup #3108

Merged
merged 3 commits into from Oct 16, 2020
Merged

Use milliseconds as an interval unit in EurekaEndpointGroup #3108

merged 3 commits into from Oct 16, 2020

Conversation

trustin
Copy link
Member

@trustin trustin commented Oct 13, 2020 •
edited

Motivation:

We almost always use milliseconds as the default unit for representing a
duration or interval. However, EurekaEndpointGroupBuilder and EurekaUpdatingServerListener
uses seconds.

Modifications:

  • Added the following methods:
    • EurekaEndpointGroupBuilder.registryFetchIntervalMillis()
    • EurekeUpdatingServerListener.leaseDurationMillis()
    • EurekeUpdatingServerListener.renewalIntervalMillis()
  • Deprecated the following methods:
    • EurekaEndpointGroupBuilder.registryFetchIntervalSeconds()
    • EurekeUpdatingServerListener.leaseDurationSeconds()
    • EurekeUpdatingServerListener.renewalIntervalSeconds()
  • Miscellaneous:
    • Improved validation for registryFetchInterval(Duration)
    • HealthCheckedEndpointGroupBuilder now overrides retryIntervalMillis().

Result:

  • Consistency
  • (Deprecation) The following methods have been deprecated in favor of their
    millis-accepting versions:
    • EurekaEndpointGroupBuilder.registryFetchIntervalSeconds()
    • EurekeUpdatingServerListener.leaseDurationSeconds()
    • `EurekeUpdatingServerListener.renewalIntervalSeconds()

@trustin
Use milliseconds as an interval unit in EurekaEndpointGroup
f8036b9 
Motivation:

We almost always use milliseconds as the default unit for representing a
duration or interval.  `EurekaEndpointGroupBuilder.registryFetchIntervalSeconds()`
is the only exception we have.

Modifications:

- Add `EurekaEndpointGroupBuilder.registryFetchIntervalMillis()` which
  deprecates `registryFetchIntervalSeconds()`
- Miscellaneous:
  - Improved validation for `registryFetchInterval(Duration)`
  - `HealthCheckedEndpointGroupBuilder` now overrides `retryIntervalMillis()`.

Result:

- Consistency
- (Deprecation) `EurekaEndpointGroupBuilder.registryFetchIntervalSeconds()`
  has been deprecated in favor of `registryFetchIntervalMillis()`.
@trustin trustin added the defect label Oct 13, 2020
@trustin trustin added this to the 1.2.0 milestone Oct 13, 2020
@trustin trustin mentioned this pull request Oct 13, 2020
Merged
@trustin
Copy link
Member Author

trustin commented Oct 13, 2020

I also find EurekaUpdatingListenerBuilder uses seconds instead of milliseconds. It's because Eureka exposes its node metadata in seconds. We have two options:

  • Accept milliseconds when building, convert it to seconds while rounding up.
  • Leave it as it is.

ikhoon
ikhoon approved these changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks! @trustin

eureka/src/main/java/com/linecorp/armeria/client/eureka/EurekaEndpointGroupBuilder.java Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 13, 2020 •
edited

Codecov Report

Merging #3108 into master will increase coverage by 0.00%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3108   +/-   ##
=========================================
  Coverage     73.90%   73.91%           
- Complexity    12305    12311    +6     
=========================================
  Files          1064     1064           
  Lines         46846    46877   +31     
  Branches       5949     5958    +9     
=========================================
+ Hits          34623    34648   +25     
+ Misses         9203     9191   -12     
- Partials       3020     3038   +18
Impacted Files Coverage Δ Complexity Δ
...eria/client/eureka/EurekaEndpointGroupBuilder.java 34.24% <25.00%> (-0.97%) 7.00 <0.00> (ø)
...a/server/eureka/EurekaUpdatingListenerBuilder.java 32.03% <64.28%> (+9.37%) 9.00 <4.00> (+3.00)
...healthcheck/HealthCheckedEndpointGroupBuilder.java 95.65% <100.00%> (+0.19%) 12.00 <1.00> (+1.00)
...orp/armeria/client/eureka/EurekaEndpointGroup.java 60.71% <100.00%> (ø) 23.00 <0.00> (ø)
...orp/armeria/server/eureka/InstanceInfoBuilder.java 71.42% <100.00%> (+3.57%) 17.00 <0.00> (+1.00)
...inecorp/armeria/common/ClosedSessionException.java 54.54% <0.00%> (-18.19%) 5.00% <0.00%> (-1.00%)
...p/armeria/common/stream/AbstractStreamMessage.java 82.85% <0.00%> (-1.91%) 16.00% <0.00%> (ø%)
...a/com/linecorp/armeria/client/HttpChannelPool.java 79.77% <0.00%> (-1.39%) 67.00% <0.00%> (-2.00%)
...inecorp/armeria/server/grpc/ArmeriaServerCall.java 84.25% <0.00%> (-1.28%) 80.00% <0.00%> (ø%)
...necorp/armeria/client/HeapBasedEventLoopState.java 92.78% <0.00%> (-1.04%) 31.00% <0.00%> (-1.00%)
... and 10 more

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 f24d613...ba1e7c3. Read the comment docs.

@trustin
Copy link
Member Author

trustin commented Oct 13, 2020

Updated EurekaUpdatingListenerBuilder to use milliseconds rather than seconds.

@ikhoon
Copy link
Contributor

ikhoon commented Oct 14, 2020

Stil LGTM

minwoox
minwoox approved these changes Oct 14, 2020
View reviewed changes
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@trustin trustin merged commit 9dc67c3 into line:master Oct 16, 2020
@trustin trustin deleted the units branch October 16, 2020 07:27
@trustin
Copy link
Member Author

trustin commented Oct 16, 2020

Thanks for reviewing!

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

@ikhoon ikhoon ikhoon approved these changes

@minwoox minwoox minwoox approved these changes

Assignees
No one assigned
Labels
defect deprecation
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@trustin @ikhoon @minwoox