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

Check current instance status before update. #1569

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

OlgaMaciaszek
Copy link
Contributor

This is related to spring-cloud/spring-cloud-netflix#4094 as an attempted fix.

@OlgaMaciaszek OlgaMaciaszek marked this pull request as ready for review February 4, 2025 16:19
@OlgaMaciaszek
Copy link
Contributor Author

@howardyuan could you please take a look?

@OlgaMaciaszek
Copy link
Contributor Author

Adding pic with comments from offline discussion:
Screenshot 2025-03-04 at 9 02 17 AM

@OlgaMaciaszek
Copy link
Contributor Author

Hi @howardyuan, regarding your comments before, I still use the pre-existing method under the hood from here, so setIsDirty will be called as well if we do change the status. I have added an exception when we verify the previous status first, as in this scenario we assume that it is the status that should be there in a correct workflow and if it's not set then possibly there was a race condition. null is returned in the pre-existing implementation if the status we want to change to is also the one already set - so nothing needs to be done. Seems like a different scenario to me. Let me know what you think. CC @spencergibb

public synchronized InstanceStatus setStatus(InstanceStatus expected, InstanceStatus status) {
InstanceStatus prev = this.status;
if (expected == null || !expected.equals(prev)) {
throw new IllegalArgumentException("Instance status mismatch");

Choose a reason for hiding this comment

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

I'm a bit concerned that we throw an exception here instead of like before which is simply returning null like before? Also, we don't setIsDirty() here anymore? not sure why we introduce some discrepancy here?

Choose a reason for hiding this comment

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

Hi @OlgaMaciaszek My concern was that even if we have a race condition, should we throw exceptions or we just skip this update and let eventual consistency catches up? I'm not sure if we will catch those exception at the top, but I think it's too much a disruption if we crash the app whenever we detect a race condition updating the instance status? lmk what you think? I'll take a further look at the whole thing but this is my quick thinking.

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.

2 participants