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 8114/stabilize natural id invalidation test #5385

Conversation

slaskawi
Copy link
Contributor

@slaskawi slaskawi commented Aug 21, 2017

https://issues.jboss.org/browse/ISPN-8114

Extracting Eventually is necessary to use it in Hibernate integration module.

@galderz
Copy link
Member

galderz commented Aug 21, 2017

@slaskawi First of all, thanks for having a go at fixing this.

However, I think your fix goes against the spirit of the test. We have put in place some latches that should met in order for test to pass. To add eventually, or similar calls, which really means, try for N seconds until this is true, goes against what Radim and I agreed in this email thread.

To be more precise, both Radim and I agreed in that thread that waiting for a certain amount of time until the condition is true would be potentially unreliable.

I'd suggest you pass this onto me and I can have a closer look to understand what's really wrong with the test.

@galderz
Copy link
Member

galderz commented Aug 21, 2017

I've submitted #5386 to ignore the test while I look into it.

@slaskawi
Copy link
Contributor Author

slaskawi commented Aug 22, 2017

@galderz Sure, no problem. Please feel free to close this PR if you wish.

But I'm really not sure if you and @rvansa are right on this particular case. Failing an assert here strongly suggests that the test progressed too quickly to let the remote listener to catch the event. Of course adding latches or other type of synchronizers might fix it but isn't that overengineering? Remote listeners are async by its nature and tests should take it into account. Just a pure KISS rule ;)

Anyway... as I said, please feel free to fix it whichever way you think is reasonable (and close this PR).

@rvansa
Copy link
Member

rvansa commented Aug 22, 2017

@slaskawi Note that the remote listener is not a clustered listener, it's just a local listener on another node (where we're not doing the main operation). I don't feel too strongly about using eventually, but something along the lines of expectEvict would be better (expectPFER? Haven't studied the test thoroughly), as you also assert that the listener is triggered by the operation you expect. Timing wise there'd be the same limit for eventually/latch.await so there would be the same amount of false positives due to time skew, but in the 'happy' case the test won't wait in the eventually loop.

@rvansa
Copy link
Member

rvansa commented Aug 22, 2017

+1 for the unification of eventually, but I would
a) use static import of the method (personal preference but we do the same for Exceptions etc. in most places)
b) use lambda instead of new Condition() { ...}

EDIT: removed suggestion to use java.util.function.BooleanSupplier, the throws declaration is useful.

@slaskawi slaskawi closed this Aug 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants