-
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
Fixes issue #93 #97 #98 #99 #100
Conversation
NiteshKant
commented
Apr 15, 2014
- Implementing clean shutdown of client. Moved away from Timer to ScheduledExecutorService as it gives better control of shutdown by interrupting the running thread, which is what we want.
- Implemented addition of "application group" to instance info.
1) Implementing clean shutdown of client. Moved away from Timer to ScheduledExecutorService as it gives better control of shutdown by interrupting the running thread, which is what we want. 2)
eureka-pull-requests #69 FAILURE |
ScheduledExecutorService part looks fine. |
eureka-pull-requests #70 SUCCESS |
@@ -267,6 +270,14 @@ public Builder setAppName(String appName) { | |||
return this; | |||
} | |||
|
|||
public Builder setAppGroupName(String appGroupName) { |
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.
Seems like the appGroupName upper case invariant can be violated here and this may be an issue since appGroupName is volatile, hinting that it may be accessed by multiple threads. A safer way to implement this function would be,
if (appGroupName == null)
result.appGroupName = null;
else
result.appGroupName = appGroupName.toUpperCase();
It appears that we have several existing methods that should also be corrected this way
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.
Although I agree with your comment in isolation, that what you suggested is a better way, but if you look at the code, this function is inside the Builder and the "result" instance it is updating is essentially "hidden" till build is called. So, unless the builder is concurrently updated, this should not be an issue. makes sense?
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 agree that this might be seen as a nitpick but we should err on the side of code correctness, especially when this is a very easy change. While we're on the subject of this builder it's important to point out that it is inherently flawed is that the builder maintains a reference to the returned InstanceInfo and can modify that object after it is returned. The builder should actually return an immutable object. Sure, this isn't an issue as long as the Builder is used correctly but can be the source of difficult to find bugs in the future.
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 agree with all you are saying, I have updated the PR.
eureka-pull-requests #71 SUCCESS |
eureka-pull-requests #72 SUCCESS |
appGrpNameFromEnv = ConfigurationManager.getConfigInstance().getString(APP_GROUP_ENV_VAR_NAME, | ||
UNKNOWN_APPLICATION); | ||
|
||
String env = ConfigurationManager.getConfigInstance().getString(EUREKA_ENVIRONMENT, TEST); |
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.
Do you mean by getString(EUREKA_ENVIRONMENT, "TEST")?
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.
No, I didn't see the TEST="test" definition above. So np here.