Skip to content

Add GRACEFUL_DISCONNECT event support (CEP-59) - #61

Open
Shanzita wants to merge 2 commits into
datastax:1.xfrom
Shanzita:cep-59
Open

Add GRACEFUL_DISCONNECT event support (CEP-59)#61
Shanzita wants to merge 2 commits into
datastax:1.xfrom
Shanzita:cep-59

Conversation

@Shanzita

Copy link
Copy Markdown

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).

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")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: GRACEFUL_DISCONNECT is only supported from v5 and above so it makes more sense to be

Suggested change
@UseDataProvider(location = TestDataProviders.class, value = "protocolV3OrAbove")
@UseDataProvider(location = TestDataProviders.class, value = "protocolV5OrAbove")

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in 62d0dfc, the test now uses protocolV5OrAbove.

@absurdfarce absurdfarce Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm not sure why we're constraining this by protocol version at all. These events exist outside of the native protocol specification; they're new events we've added locally and (in theory) someone could implement support for this using any native protocol version.

I'm not saying we should make or test this for earlier native protocol versions... I'm just saying the two things aren't explicitly related.

Maybe we should change this constraint to say something like "this test should only be run against this version of Cassandra or higher since that's the version when graceful disconnect support was added"? Or am I missing something (entirely possible)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my earlier comment wasn't super-clear. I understand that we're constraining this by protocol version because we're using test providers to get test input data here. A better way to say what I was trying to get at above is that ideally this test would run through all native protocol versions because it's ability to pass or fail should be completely independent of a given protocol version... and testing against all of them (or as many as possible) is the only way I can think of to simulate that.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing against all protocol versions makes sense to me. @Shanzita Please go ahead and change the test to be against all versions.

@SiyaoIsHiding SiyaoIsHiding left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@absurdfarce

Copy link
Copy Markdown
Contributor

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.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants