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) |
| @@ -183,7 +183,11 @@ public void testProxyConnectionDisconnect() throws Exception { | |||
| } | |||
| } | |||
| }); | |||
| 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).
Polling `Future.isDone` is obviously nonesense. Suprisingly enough, We can save a couple seconds fixing up these spots (especially the CCS ones).
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.