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.

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 1098 #1100

Merged
merged 3 commits into from
Aug 2, 2018
Merged

Fix 1098 #1100

merged 3 commits into from
Aug 2, 2018

Conversation

holy12345
Copy link
Contributor

Hi @qiangdavidliu

This PR fix #1098 please check it.

Thanks : )

@qiangdavidliu
Copy link
Contributor

@holy12345 thanks for the PR. This looks good in theory, though I would like to get some additional verification on whether this is safe to do as removal of apps is not something that expected in current client behaviour. Can you please add some testing to verify? DiscoveryClientRegistryTest would be a good starting point for this. Thanks.

@holy12345
Copy link
Contributor Author

@qiangdavidliu Great thanks : )

I will add some test for this PR.
One more things could you please give me your personal email? If you think it inconvenient to disclose it in public, I can give you my email address(Because I want more discuss with you about eureka).

My email address is spring_holy@163.com

Thanks again

@holy12345
Copy link
Contributor Author

Hi @qiangdavidliu First of all thank you!

I add some test case for this PR, please you check it .

You say

removal of apps is not something that expected in current client behaviour

So I think in some case there have many application has no instanceInfo when merge the delta information(the action type is DELETE)

According to your thoughts which is best ?

  • When application has no instanceInfo, we remove application(this PR logic)

  • Optimize the original logic, the code like this

if (ActionType.DELETED.equals(instance.getActionType())) {
    Application existingApp = applications.getRegisteredApplications(instance.getAppName());
    if (existingApp != null) {
        logger.debug("Deleted instance {} to the existing apps ", instance.getId());
        applications.getRegisteredApplications(instance.getAppName()).removeInstance(instance);
    }
}

Thanks again : )

@qiangdavidliu
Copy link
Contributor

Looks good, thanks @holy12345 .

@qiangdavidliu qiangdavidliu merged commit 6c7596f into Netflix:master Aug 2, 2018
@holy12345 holy12345 deleted the fix-1098 branch August 7, 2018 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DiscoveryClient.localRegionApps not clear application without any instances
2 participants