Add GRACEFUL_DISCONNECT event support (CEP-59) - #61
Conversation
Adds the GRACEFUL_DISCONNECT event type and its codec, allowing clients to be notified in-band when a node begins draining connections for shutdown. The event has an empty body; the type string alone carries the signal. Part of CEP-59: Graceful Disconnect - In-Band Connection Draining for Node Shutdown. Driver-side handling: CASSJAVA-124.
| } | ||
|
|
||
| @Test | ||
| @UseDataProvider(location = TestDataProviders.class, value = "protocolV3OrAbove") |
There was a problem hiding this comment.
nit: GRACEFUL_DISCONNECT is only supported from v5 and above so it makes more sense to be
| @UseDataProvider(location = TestDataProviders.class, value = "protocolV3OrAbove") | |
| @UseDataProvider(location = TestDataProviders.class, value = "protocolV5OrAbove") |
There was a problem hiding this comment.
Good catch — fixed in 62d0dfc, the test now uses protocolV5OrAbove.
There was a problem hiding this comment.
I wondered about this too but this isn't really the case, is it? The spec has always allowed for other messages/events to be published, presumably so that other implementations of native-protocol could add on additional functionality. In fact that's why we're able to do this without requiring a new protocol version. So why would we constrain this to v5 only?
There was a problem hiding this comment.
This worth a discussion for sure. My reasoning:
Supporting v4:
- Pro: very limited. It will only bring benefits if a component does not support protocol v5 but support graceful disconnect. Currently Java driver, python driver, GoCQL, Node.js driver already support v5, so what's the chance of other components, e.g. Ruby driver, to support graceful disconnect before suppoting v5..... Almost none I'd say.
- Con: Costs almost nothing to implement, basically just a line of code in each component. But we would get a version that we have to take on the responsibility to support.
I'd prefer not supporting v4 because I think the benefit is trivial. I'm open to either, tho.
What do you think?
SiyaoIsHiding
left a comment
There was a problem hiding this comment.
Generally LGTM!
One concern about the specs in src/main/resources/. @absurdfarce How did we/should we keep those in sync with the server side spec? Do we change the CQL protocol spec here first, then make sure the server side PR align with ours?
GRACEFUL_DISCONNECT is only supported from protocol v5, per review feedback.
|
Good question @SiyaoIsHiding. The specs in src/main/resources are out-of-date and need a general overall update (that's what #46 aims to fix). But since this work isn't adding a new spec or changing any of the existing ones we shouldn't need to update anything for this PR. |
Adds the GRACEFUL_DISCONNECT event type and its codec, allowing clients to be notified in-band when a node begins draining connections for shutdown. The event has an empty body; the type string alone carries the signal.
Part of CEP-59: Graceful Disconnect - In-Band Connection Draining
for Node Shutdown. Driver-side handling: CASSJAVA-124.
Testing : mvn clean verify — 595 tests, 0 failures (JDK 8).