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

ISPN-13612 InvalidatedNearRemoteCache can persist stale values in cas… #11195

Merged
merged 1 commit into from Aug 9, 2023

Conversation

wburns
Copy link
Member

@wburns wburns commented Aug 7, 2023

@wburns wburns added the 14.0.x Annotate a PR with this label if you want it to be backported to the 14.0.x branch label Aug 7, 2023
Copy link
Member

@jabolina jabolina left a comment

Choose a reason for hiding this comment

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

Looks good. Just one question.

Comment on lines 76 to 77
MetadataValue<V> nearValue = nearcache.get(key);
if (nearValue == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Could a concurrent get retrieve the placeholder here? Should we extend the if check?

Copy link
Member Author

@wburns wburns Aug 7, 2023

Choose a reason for hiding this comment

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

Yes, it can... I forgot to add || nearValue.value() == null. Good catch.

@wburns wburns force-pushed the ISPN-13612_near_cache_stale branch from b7b9b57 to 5c6fe3c Compare August 7, 2023 21:30
@wburns
Copy link
Member Author

wburns commented Aug 7, 2023

Updated to fix one of the comments, unfortunately I am not sure of a better way then a sleep without making the test very complex to fix the timing issue :(

Copy link
Member

@pruivo pruivo left a comment

Choose a reason for hiding this comment

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

Shouldn't client/hotrod be changed too?

@wburns
Copy link
Member Author

wburns commented Aug 8, 2023

Shouldn't client/hotrod be changed too?

Yes, I guess I erroneously thought you couldn't use it in the new client, but looking at the code closer I was wrong :D

@wburns wburns force-pushed the ISPN-13612_near_cache_stale branch from 5c6fe3c to 66c37e0 Compare August 8, 2023 19:26
@wburns
Copy link
Member Author

wburns commented Aug 8, 2023

Review comments are addressed as I could. Just adding documentation note as I forgot to push before.

@wburns wburns force-pushed the ISPN-13612_near_cache_stale branch from 66c37e0 to 90915d5 Compare August 8, 2023 19:46
@wburns
Copy link
Member Author

wburns commented Aug 8, 2023

Updated

Copy link
Member

@pruivo pruivo left a comment

Choose a reason for hiding this comment

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

LGTM, just a bunch of nitpicks 🤣

@wburns wburns force-pushed the ISPN-13612_near_cache_stale branch from 90915d5 to 03acc1c Compare August 9, 2023 14:35
@wburns
Copy link
Member Author

wburns commented Aug 9, 2023

Updated trace messages.

@pruivo pruivo merged commit a425875 into infinispan:main Aug 9, 2023
3 of 4 checks passed
@pruivo
Copy link
Member

pruivo commented Aug 9, 2023

merged! thanks @wburns !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
14.0.x Annotate a PR with this label if you want it to be backported to the 14.0.x branch
Projects
None yet
3 participants