Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

Data encoder config #429

Merged
merged 7 commits into from
Feb 25, 2022
Merged

Data encoder config #429

merged 7 commits into from
Feb 25, 2022

Conversation

robholland
Copy link
Contributor

What was changed

Refactored the remote data encoder code a little to be more consistent with terms. Also added some documentation around the setup of such a system, although this should be improved and centralised later.

Why?

This allows users to configure a remote data encoder endpoint globally for the Temporal Web instance rather than relying on per-session settings as was previously the case. Given remote data encoders will be deployed and shared it does not make sense to require per-session settings for every user.

Checklist

  1. How was this tested:
    Tested using my background-checks integration branch.

  2. Any docs updates needed?
    Yes, docs will need to involve as more SDKs add support for remote data encoding and we add a central page that documents the setup as a whole.

Later this will probably be adjusted to point to a central docs page that explains the full setup, including security implications.
@robholland
Copy link
Contributor Author

Updated to latest protocol so now relies on: temporalio/sdk-go#733

}

requests.push(
fetch(`${endpoint}/decode`, { method: 'POST', headers: headers, body: JSON.stringify(payloadsWrapper) })

Choose a reason for hiding this comment

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

Just a thought we should make sure wherever this is happening has http2.1 otherwise we'll only be able to decode 8 things at a time which would severely block rendering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The server aspect will be up to the users, it's not something we control. We can document that it should support http2 though.

Copy link

@GiantRobots GiantRobots left a comment

Choose a reason for hiding this comment

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

I think that data encoder is a better name (and I think makes it easier to talk about) but we should double check our docs to ensure that we're not shooting ourselves in the foot for confusion with our users. If we do decide to do that we should loop docs in to update some pages as well.

@robholland
Copy link
Contributor Author

Yep, I'll be working on the docs for all this and will make sure the difference is clear between Data Converter and Remote Encoder.

@robholland robholland merged commit b64f163 into master Feb 25, 2022
@robholland robholland deleted the rh-data-encoder-config branch February 25, 2022 09:09
feedmeapples pushed a commit that referenced this pull request Feb 25, 2022
* Wire up remote data encoder config.

* Update to latest remote decoder protocol.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants