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

Change the order in which events are issued #4274

Merged
merged 4 commits into from
Apr 9, 2024

Conversation

heowc
Copy link
Contributor

@heowc heowc commented Mar 29, 2024

The order in which events are issued should be after the desired processing, but they are preceded.

@spencergibb
Copy link
Member

Can you describe the problem you are facing that drove you to propose these changes?

@heowc
Copy link
Contributor Author

heowc commented Mar 30, 2024

Can you describe the problem you are facing that drove you to propose these changes?

@spencergibb hello,

I am writing a PR that collects metrics using these events. However, the information reflected in the registry is unknown when the event occurs. This is because events are issued first before being registered/deleted.

@EventListener
public void onEvent(EurekaInstanceRegisteredEvent event) {
    final InstanceInfo instanceInfo = event.getInstanceInfo();
    instanceRegistry.getInstanceByAppAndId(instanceInfo.getAppName(), instanceInfo.getInstanceId());  <--
}

Also, I find it somewhat odd that events are issued before register/delete actions. Isn't that right?

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @heowc. Have added a comment. Please address.

@heowc heowc requested a review from OlgaMaciaszek April 4, 2024 15:34
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks, @heowc. Looks good, but have added some cosmetic comments. Please take a look.

@heowc heowc requested a review from OlgaMaciaszek April 8, 2024 12:04
@heowc
Copy link
Contributor Author

heowc commented Apr 9, 2024

@OlgaMaciaszek,

Build failed. 😭
But it works fine on my local.

Is this a flaky test?

https://github.com/spring-cloud/spring-cloud-netflix/actions/runs/8599692396/job/23571366157?pr=4274#step:4:25977

@OlgaMaciaszek
Copy link
Collaborator

OlgaMaciaszek commented Apr 9, 2024

@heowc It's not a flaky test - it's directly caused by the changes made. We are now checking for a boolean result of those operations that we're not getting (the test is not set up to run the full operation including communicating with an Eureka Server). You can stub it appropriately in your tests, but make sure to verify the behaviour against a running Eureka server in your local setup. It's bizarre that it passes for you locally - maybe some old code version cached somewhere?

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Please adjust tests to code changes.

@heowc
Copy link
Contributor Author

heowc commented Apr 9, 2024

Please adjust tests to code changes.

Fixed 🤣

@heowc heowc requested a review from OlgaMaciaszek April 9, 2024 09:56
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks @heowc, please see the comments and fix them.

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

LGTM.

@OlgaMaciaszek OlgaMaciaszek merged commit dbb36fb into spring-cloud:main Apr 9, 2024
1 check passed
@heowc heowc deleted the feature/3685-1 branch April 9, 2024 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants