Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce protocol versioning #421

Merged
merged 6 commits into from
May 30, 2023
Merged

Introduce protocol versioning #421

merged 6 commits into from
May 30, 2023

Conversation

flyingsilverfin
Copy link
Member

@flyingsilverfin flyingsilverfin commented May 27, 2023 •
edited
Loading

What is the goal of this PR?

We use a new protocol API to perform a "connection open". This API does server-side protocol version compatibility checks, and replaces our previous need to get all databases to check that the connection is available.

The user will receive an error about a protocol version mismatch if they are using a client-server combination that are not using exactly compatible protocols.

  1. The server will raise an error if the client tries to connect with a mismatching protocol version
  2. The client will raise an error if it tries to connect to the server and the server does not have that API

Both errors imply a client-server mismatch and the error will suggest this as a fix.

This change depends on typedb/typedb-protocol#185, which means that this client will implement protocol version 1.

What are the changes implemented in this PR?

  • Refactor validateConnectionOrThrow into openConnection, was then inlined into the constructors of CoreClient and ClusterServerClient
  • Call the new openConnection with the protocol Version set when constructing a client
  • Throw a useful message if the server returns an unimplemented error

@typedb-bot
Copy link
Member

PR Review Checklist

Do not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed.

Trivial Change

  • This change is trivial and does not require a code or architecture review.

Code

  • Packages, classes, and methods have a single domain of responsibility.
  • Packages, classes, and methods are grouped into cohesive and consistent domain model.
  • The code is canonical and the minimum required to achieve the goal.
  • Modules, libraries, and APIs are easy to use, robust (foolproof and not errorprone), and tested.
  • Logic and naming has clear narrative that communicates the accurate intent and responsibility of each module (e.g. method, class, etc.).
  • The code is algorithmically efficient and scalable for the whole application.

Architecture

  • Any required refactoring is completed, and the architecture does not introduce technical debt incidentally.
  • Any required build and release automations are updated and/or implemented.
  • Any new components follows a consistent style with respect to the pre-existing codebase.
  • The architecture intuitively reflects the application domain, and is easy to understand.
  • The architecture has a well-defined hierarchy of encapsulated components.
  • The architecture is extensible and scalable.

@flyingsilverfin flyingsilverfin changed the title Protocol version Protocol versioning May 27, 2023
@flyingsilverfin flyingsilverfin changed the title Protocol versioning Introduce protocol versioning May 27, 2023
@flyingsilverfin flyingsilverfin self-assigned this May 27, 2023
@flyingsilverfin flyingsilverfin marked this pull request as ready for review May 27, 2023 09:20
@flyingsilverfin flyingsilverfin merged commit f859748 into typedb:master May 30, 2023
@flyingsilverfin flyingsilverfin deleted the protocol-version branch May 30, 2023 13:51
alexjpwalker
alexjpwalker reviewed May 30, 2023
View reviewed changes
common/exception/ErrorMessage.java
Comment on lines +31 to +32
public static final Client RPC_METHOD_UNAVAILABLE =
new Client(1, "The server does not support this method, please check the client-server compatibility:\n'%s'.");
Copy link
Member

Choose a reason for hiding this comment

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

This error message could be friendlier, something like
"The server does not support this method. Please ensure that the TypeDB Client and TypeDB Server versions are compatible ..."

Copy link
Member Author

Choose a reason for hiding this comment

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

Will push an update, thank you!

common/exception/TypeDBClientException.java
@@ -85,6 +93,11 @@ private static boolean isPasswordCredentialExpired(StatusRuntimeException status
statusRuntimeException.getStatus().getDescription() != null &&
statusRuntimeException.getStatus().getDescription().contains(CLUSTER_PASSWORD_CREDENTIAL_EXPIRED_ERROR_CODE);
}

private static boolean isUnimplementedMethod(StatusRuntimeException statusRuntimeException) {
return statusRuntimeException.getStatus().getCode() == Status.Code.UNIMPLEMENTED;
Copy link
Member

Choose a reason for hiding this comment

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

This is a sufficient but not necessary condition for a client-server mismatch error. It's also possible that we send a request message with an RPC code that is implemented, but carries a different semantic meaning in this protocol version.

Do we perform some kind of handshake anywhere to verify the protocol version? I notice OpenReq now sends a version number. Does that mean the server will decline a connection request from an invalid client?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, and it throws basically the equivalent error message if it is a mismatch :) between the two of them we should get pretty good coverage of client-protocol mismatches -> useful errors the users can resolve by themselves.

@flyingsilverfin flyingsilverfin mentioned this pull request Jul 2, 2023
Closed
@dmitrii-ubskii dmitrii-ubskii mentioned this pull request Aug 9, 2023
Merged
dmitrii-ubskii added a commit that referenced this pull request Aug 22, 2023
@dmitrii-ubskii @benjaminasmall
@flyingsilverfin @krishnangovindraj
Implement expressions and top-level values in client-rust (#434)
5fdec21 
## What is the goal of this PR?

We update the Rust driver to support the features introduced in driver
Java since v2.19 in effort to preserve the feature set when
transitioning from the JVM-native implementation to the Rust JNI
implementation.

## What are the changes implemented in this PR?

Changes in master since branching (#417):

Reimplemented in Rust and made available in Java over JNI:
- #409 
- #421 
- 1f396a6 Improve method unavailable error message
- #430 
- #431: partial, since `tonic` does not report SSL errors to the same
granularity

Cherry-picked directly:
- #415
- 42800e7 Update VERSION to 2.18.1
- 7cd0a5a Add eclipsesource-minimal-json to maven dependencies (#423)
- 63614ec Update CODEOWNERS
- #424

Dropped entirely (Java-specific or obsolete):
- #422 
- 8c0caa1 Update VERSION and release notes

---------

Co-authored-by: Benjamin Small <benjaminasmall@gmail.com>
Co-authored-by: joshua <joshua@vaticle.com>
Co-authored-by: Krishnan Govindraj <krishnangovindraj@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexjpwalker alexjpwalker alexjpwalker left review comments

@haikalpribadi haikalpribadi Awaiting requested review from haikalpribadi

Assignees

@haikalpribadi haikalpribadi

@flyingsilverfin flyingsilverfin

@alexjpwalker alexjpwalker

Labels
priority: high type: feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@flyingsilverfin @typedb-bot @alexjpwalker @haikalpribadi