-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fix DefaultEurekaClientConfig units #107
Conversation
eureka-pull-requests #91 SUCCESS |
This change will break existing config files that specify serviceUrlPollIntervalMs, or that specify either eurekaServer.readTimeout or eurekaServer.connectTimeout as milliseconds, not seconds. To fix the inconsistencies either the config files must be changed or the interface must be changed. I have changed the config files here, but I can submit the other if that is preferred. |
@@ -132,7 +132,7 @@ public int getInstanceInfoReplicationIntervalSeconds() { | |||
@Override | |||
public int getEurekaServiceUrlPollIntervalSeconds() { | |||
return configInstance.getIntProperty( | |||
namespace + "serviceUrlPollIntervalMs", 5 * 60 * 1000).get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its better to keep this property name same and convert it into seconds.
eureka-pull-requests #94 FAILURE |
This is passing tests locally. Note that this will still break with config files that specify either eurekaServer.readTimeout or eurekaServer.connectTimeout as milliseconds, not seconds. |
EurekaServiceUrlPollInterval is named getEurekaServiceUrlPollIntervalSeconds in EurekaClientConfig interface but serviceUrlPollIntervalMs in config files. Its default looks suspiciously like it shouldn't be multiplied by 1000. This config is actually used as seconds. Changed to convert the config serviceUrlPollIntervalMs to into seconds. EurekaServerReadTimeoutSeconds and EurekaServerConnectTimeoutSeconds were both being contributed as millseconds and used as milliseconds. Changed to seconds in config files and use of the config changed to use seconds.
eureka-pull-requests #95 SUCCESS |
Yes that is reasonable as we are atleast being truthful to what the property name is :) |
Fix DefaultEurekaClientConfig units
EurekaServiceUrlPollInterval is named
getEurekaServiceUrlPollIntervalSeconds in EurekaClientConfig interface
but serviceUrlPollIntervalMs in config files. Its default looks
suspiciously like it shouldn't be multiplied by 1000. This config is
actually used as seconds. Changed to be seconds
serviceUrlPollIntervalSeconds and fixed the default.
EurekaServerReadTimeoutSeconds and EurekaServerConnectTimeoutSeconds
were both being contributed as millseconds and used as milliseconds.
Changed to seconds in config files and use of the config changed to use
seconds.