Remove needless use of assertBusy polling Future.isDone #119078
Conversation
Polling `Future.isDone` is obviously nonesense. Suprisingly enough, We can save a couple seconds fixing up these spots (especially the CCS ones).
original-brownbear added >non-issue :Search/Search 
| Pinging @elastic/es-search (Team:Search) |
| }); | ||
| assertBusy(() -> assertTrue(future.isDone())); | ||
| try { | ||
| future.get(); |
There was a problem hiding this comment.
Should you use a timed get to avoid an infinite wait scenario?
There was a problem hiding this comment.
Good question :) "infinite wait" practically means 20min with the suite timeout we have in place.
To be honest, I kinda like not putting timeouts on these things personally. If the listener leaks due to a bug, yes CI will run for 20 min, but so be it? :) If I fail after a 20 min wait, I'm close to 100% sure it wasn't a slow CI run that failed things. Otoh, if it's a 10s wait ... the first thing my mind will jump to is wonder if we conceivably could have had have a 10s GC or so pause here :)
-> personally I wouldn't add timeouts on future.get() in tests anywhere pretty much I think.
original-brownbear added auto-backport Polling `Future.isDone` is obviously nonesense. Suprisingly enough, We can save a couple seconds fixing up these spots (especially the CCS ones).
4 participants
Polling
Future.isDoneis obviously nonsense. Surprisingly, We can save a couple seconds fixing up these spots (especially the CCS ones).Seemingly this also fixes
testCancel(tested by unmuting) but I left it muted for now to be investigated in the dedicated issue for that.Adding search label since most of the fixed tests are from search.