spring-cloud / spring-cloud-netflix

Integration with Netflix OSS components
http://cloud.spring.io/spring-cloud-netflix/
Apache License 2.0
4.87k stars 2.44k forks source link

Change the order in which events are issued #4274

Closed heowc closed 5 months ago

heowc commented 6 months ago

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

spencergibb commented 6 months ago

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

heowc commented 6 months ago

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?

heowc commented 6 months ago

@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 commented 5 months ago

@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?

heowc commented 5 months ago

Please adjust tests to code changes.

Fixed 🤣