-
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 Eureka instance count and status logging #130
Conversation
Eureka client would only log the number of UP instances but with the deceiving message: logger.debug("The total number of instances in the client now is {}", totInstances);
The log message is deceiving as-is. Another fix would be to change the log message to be: logger.debug("The total number of UP instances in the client now is {}", totInstances); Let me know which you would prefer. |
eureka-pull-requests #132 FAILURE |
eureka-pull-requests #133 SUCCESS |
eureka-pull-requests #134 FAILURE |
A few points to make about the "Fix appsHashCode not getting updated" commit.
|
appsHashCode was not getting updated under 2 circumstances. First when a BackupRegistry is used at all. Second when fetchRegistry is called. Most notably when this is called by the CacheRefreshThread.
eureka-pull-requests #135 SUCCESS |
@pgkelley4 apologies for the late reply.
In the DiscoveryClient, the app hash code used to always come from the server (in the response) but looks like after the remote region change, this hash code was not getting updated to reflect the remote instances.
Sure it can be.
Yes thats a valid point and we have thought about it before. You are correct that this scenario can occur and will incorrectly indicate that the registry contents are identical. |
The changes look good to me. Should I merge it or you have some more changes to make? |
My first point was more about when/where AppsHashCode should be set. I followed the pattern already in the code, but I really think it would be cleaner if it was managed by the Applications class, where it is stored. Then anytime the apps changed the AppsHashCode would be updated. This is a bit tricker because if someone changes instances through Application and not Applications, then AppHashCode would still have to be updated. Either way at this point that is something long term to consider. Please merge this immediate fix. |
Fix Eureka instance count and status logging
@pgkelley4 sure if you have thoughts on how to fix it, feel free to issue a PR or open an issue to discuss. |
Eureka client would only log the number of UP instances but with the
deceiving message:
logger.debug("The total number of instances in the client now is {}",
totInstances);