Skip to content

Conversation

@alexshtin
Copy link
Contributor

@alexshtin alexshtin commented Sep 25, 2020

What changed?
The way how client and server checks each other versions.

Why?
Previous schema was complicated and hard to understand/explain. This implementation follow simple rules:

  1. All validation logic is on server. Client only sends headers. If header is missing assume that "client doesn't care" and no restriction is applied.
  2. Client sends to server following headers:
    1. client-name: one of temporal-go, temporal-java, temporal-cli, or temporal-server.
    2. client-version: 0.30.0
    3. supported-server-versions: >1.1.1 <=1.4.0||^5.0.0 (https://github.com/blang/semver#ranges).
  3. Server has GetClusterInfo API which returns its version and supported version ranges for every supported client.

How did you test it?
Modified existing tests.

Potential risks
No risks, because version check wasn't actually used and because missing headers are treated as "accepted" all combinations of old/new server and old/new clients should work.

CancelRequestId string
StickyTaskQueue string
StickyScheduleToStartTimeout *time.Duration
ClientLibraryVersion string
Copy link
Member

Choose a reason for hiding this comment

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

Why are we removing it? Looks like useful info. Obviously, field names should change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussion with @samarabbas we decided to store client version info later in workflow history as version marker. Storing only last client version here doesn't make much sense.

Comment on lines +41 to +42
ServerVersion = "0.31.0"
CLIVersion = "0.31.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These should be updated with every release.

Comment on lines +50 to +53
ClientNameGoSDK: "<2.0.0",
ClientNameJavaSDK: "<2.0.0",
ClientNameCLI: "<2.0.0",
ClientNameServer: "<2.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assumes that version 2.0.0 will break compatibility.

// Version is the version associated with this build.
Version = "unknown"
// GitVersion is the git version (tag) associated with this build.
GitVersion = "unknown"
Copy link
Contributor Author

@alexshtin alexshtin Sep 25, 2020

Choose a reason for hiding this comment

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

This version has an issue. If someone just pull master every time w/o fetching tags, this version becomes stale and never gets updated.

temporal.server.api.cluster.v1.MembershipInfo membership_info = 2;
map<string,string> supported_clients = 1;
string server_version = 2;
temporal.server.api.cluster.v1.MembershipInfo membership_info = 3;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Breaking change.

Comment on lines +298 to +300
reserved 48;
reserved 49;
reserved 50;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This info is not persisted anymore as it is not used by code. @samarabbas, I am wondering if we still want to persist client name and version for debugging purpose.

clientName := headers[0]
clientVersion := headers[1]
supportedServerVersions := headers[2]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bellow is the core logic according to spec.

@alexshtin
Copy link
Contributor Author

Run manual backward compatibility tests: old canary -> new server, new canary -> old server. New canary -> new server also works :-).

@alexshtin alexshtin merged commit 3bdf1aa into temporalio:master Sep 26, 2020
@alexshtin alexshtin deleted the feature/client-server-version branch September 26, 2020 01:08
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.

3 participants