Skip to content

Client: Don't panic on eviction#2484

Merged
sentientwaffle merged 6 commits into
mainfrom
dj-client-eviction-nonfatal
Nov 18, 2024
Merged

Client: Don't panic on eviction#2484
sentientwaffle merged 6 commits into
mainfrom
dj-client-eviction-nonfatal

Conversation

@sentientwaffle

@sentientwaffle sentientwaffle commented Nov 15, 2024

Copy link
Copy Markdown
Member

Language clients no longer panic when they receive an eviction message from the cluster.
Instead, any in-flight (or future) packets fail with PacketStatus.ClientEvicted.

Note that for simplicity the client eviction reason is not made available.

Rather than plumbing the eviction reason around (e.g. by turning PacketStatus into a union(enum)?), we can use logging to inform the operator about the details.

@sentientwaffle sentientwaffle force-pushed the dj-client-eviction-nonfatal branch from 1221eb6 to 3fbe091 Compare November 15, 2024 20:56
@sentientwaffle sentientwaffle force-pushed the dj-client-eviction-nonfatal branch from 3fbe091 to 976d6c9 Compare November 15, 2024 22:16
@sentientwaffle sentientwaffle force-pushed the dj-client-eviction-nonfatal branch from 976d6c9 to 3fffb98 Compare November 15, 2024 22:23
@sentientwaffle sentientwaffle force-pushed the dj-client-eviction-nonfatal branch from 3fffb98 to 676db38 Compare November 15, 2024 22:40
@sentientwaffle sentientwaffle marked this pull request as ready for review November 15, 2024 23:06

@batiati batiati left a comment

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.

Note that for simplicity the client eviction reason is not made available.

Rather than plumbing the eviction reason around (e.g. by turning PacketStatus into a union(enum)?), we can use #2472 to inform the operator about the details.

Alternatively, we could select a few eviction reasons that are worth exposing to the client and represent them as distinguished PacketStatus errors.
For example:

const PacketError = error{
    TooMuchData,
    ClientShutdown,
    ClientReleaseIncompatible, // Evicted due to incompatible releases, should upgrade.
    ClientEvicted, // The server kicked us, we may try connecting again.
    InvalidOperation,
    InvalidDataSize,
};

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.

Note to self: RequestException does indicate user-facing exceptions such as TooMuchData, ClientShutdown and now ClientEvicted, on the contrary to what is stated in the comment "thrown when the internal invariants of the underlying native library are violated, which is expected to never happen."

This documentation must be updated, but maybe this exception should be a checked exception explicitly handled by the user, reverting 5c93c3c.

/**
* RequestException is thrown when the internal invariants of the underlying native library are
* violated, which is expected to never happen. If tis exception is thrown, then either there is a
* programming error in the tigerbeetle-java library itself, or there is a version mismatch between
* the java code and the underlying native library.
*/
public final class RequestException extends RuntimeException {

}

@Test
public void testClientEvicted() throws Throwable {

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.

❤️

@sentientwaffle

Copy link
Copy Markdown
Member Author

Alternatively, we could select a few eviction reasons that are worth exposing to the client and represent them as distinguished PacketStatus errors.

Great suggestion! I will implement that in a follow-up PR, so that this PR is included in monday's release.

@sentientwaffle sentientwaffle added this pull request to the merge queue Nov 16, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 16, 2024
@sentientwaffle

Copy link
Copy Markdown
Member Author

Looks like this PR introduced a flake to the integration tests, so it won't be in monday's release after all. I'll investigate after the weekend!

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.

2 participants