Skip to content

Conversation

@wchargin
Copy link
Contributor

Summary:
This will enable servers to detect when clients are using an old version
and send instructions to show the user how to upgrade, especially in the
case of known bugs.

We could implement this with gRPC’s client-side interceptor API, but
that API is loudly disclaimed as experimental, so I’ve gone with the
manual plumbing for now.

Test Plan:
Ran the new uploader with an existing server and verified that it still
works (extra metadata is safely ignored). Verified that a server can
be modified to read this metadata for all three flows (upload, delete,
export). Tested in both Python 2 and Python 3.

wchargin-branch: uploader-version

Summary:
This will enable servers to detect when clients are using an old version
and send instructions to show the user how to upgrade, especially in the
case of known bugs.

We could implement this with gRPC’s client-side interceptor API, but
that API is loudly disclaimed as experimental, so I’ve gone with the
manual plumbing for now.

Test Plan:
Ran the new uploader with an existing server and verified that it still
works (extra metadata is safely ignored). Verified that a server can
be modified to read this metadata for all three flows (upload, delete,
export). Tested in both Python 2 and Python 3.

wchargin-branch: uploader-version
Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

LGTM, though I think the version protection would still be a bit more robust if we can do it against an stable discovery endpoint that's called before any RPCs are sent, since that gives us the ability to remove RPC methods entirely, whereas here if they're removed I don't think we're able to check the metadata first? (Or at least it's a bit harder to implement since we'd need some lower level interception on the server side.)

@wchargin
Copy link
Contributor Author

Yes; we may well want to add such an endpoint. One nice thing about this
formulation is that it handles the case where an uploader runs for a
long period of time during which the server version changes (and the
gRPC channel transparently reconnects). Some more discussion internally
at http://b/142802157.

@wchargin wchargin merged commit cd8a471 into master Oct 27, 2019
@wchargin wchargin deleted the wchargin-uploader-version branch October 27, 2019 00:26
wchargin added a commit to wchargin/tensorboard that referenced this pull request Oct 29, 2019
Summary:
This will enable servers to detect when clients are using an old version
and send instructions to show the user how to upgrade, especially in the
case of known bugs.

We could implement this with gRPC’s client-side interceptor API, but
that API is loudly disclaimed as experimental, so I’ve gone with the
manual plumbing for now.

Test Plan:
Ran the new uploader with an existing server and verified that it still
works (extra metadata is safely ignored). Verified that a server can
be modified to read this metadata for all three flows (upload, delete,
export). Tested in both Python 2 and Python 3.

wchargin-branch: uploader-version
@wchargin wchargin mentioned this pull request Oct 29, 2019
wchargin added a commit that referenced this pull request Oct 29, 2019
Summary:
This will enable servers to detect when clients are using an old version
and send instructions to show the user how to upgrade, especially in the
case of known bugs.

We could implement this with gRPC’s client-side interceptor API, but
that API is loudly disclaimed as experimental, so I’ve gone with the
manual plumbing for now.

Test Plan:
Ran the new uploader with an existing server and verified that it still
works (extra metadata is safely ignored). Verified that a server can
be modified to read this metadata for all three flows (upload, delete,
export). Tested in both Python 2 and Python 3.

wchargin-branch: uploader-version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants