Ksql 5716/close transient queries when network splits

[swist]

Description

We've observed a behaviour where if there are two concurrent PushQueries running against a KSQL cluster, upon terminating the websocket in the browser, one of the queries lingers.

Testing done

This is a right pain to test because everything useful gets mocked, so I'm more than happy to take pointers about testing this unit-test wise. I have tested that the behaviour is now correct locally by doing the following:

1. deploy ksqldb, c3 and confluent platform
2. set up a socat relay between the frontend and ksqldb
3. modify frontend to point at the relay instead of ksqldb
4. open two queries in separate tabs
5. kill socat to simulate a network split

Prior to the change one of the queries would always stay when running show queries from another client. After the change it does not. Body of commit 12fb072 has a decent explanation why this might be the case

Reviewer checklist

• Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
• Ensure relevant issues are linked (description should include text like "Fixes #")

[ghost]

@confluentinc It looks like @swist just signed our Contributor License Agreement. 👍

Always at your service,

clabot

[colinhicks]

@swist is it feasible to add test coverage in io.confluent.ksql.rest.server.resources.WSQueryEndpointTest? Something like calling executeStreamQuery twice, attaching a test close handler to each. Then, close the second websocket before the first, and assert that both close handlers were called?

[spena]

I recommend using the log.debug parameters to pass variables to the string. Like:

log.debug("Websocket {} closed, reason: {},  code: {})", 
        websocket.textHandlerID(), 
        websocket.closeReason(), 
        websocket.closeStatusCode());

Passing the variables to the LOG instead of using the String.format() has the benefit that if DEBUG is disabled (in most cases), then the string will not be formatted. However, if using String.format(), then the string is always formatted even if the log doesn't print it because debug is disabled.

[swist]

Ah yes, was looking for something like this!

[spena]

can subscriber be null? I see that validation in the code you removed. Should it be checked before creating the closeHandler?

[swist]

There isn't really a path that makes it null, but it makes sense to be more defensive there, I'll bring back the check!

[vcrfxia]

Thanks for the fix @swist ! LGTM pending the comments Colin and Sergio have left.

[spena]

lgtm

[swist]

I'm going to merge this without the test for the private method behaviour, but will have a look at refactoring the websocket code a bit later