Add claim check recipe. #9
Conversation
Make sure we mention the codec server in the README as well as the recipe.
|
|
There was a problem hiding this comment.
I pushed some changes to the README to clarify that this for both large payloads and for compliance.
A few of notes for discussion:
- Does this fail without the Claim Check codec?
- Could we make the example more AI-relevant?
- Should we add S3 and PostgreSQL/SQLAlchemy to this recipe, or should we do that separately?
- Should we add compression to this recipe?
|
Also, we should discuss lifecycle management, e.g., how to set the TTL for Redis. |
robholland
force-pushed
the
rh-claimcheck
branch
from
31ba805 to
064de11
Compare
|
I don't think including S3 or Postgres adds any value. The codec system needs the simplest of PUT/GET style operations from whichever datastore is used. |
drewhoskins-temporal
left a comment
drewhoskins-temporal
left a comment
There was a problem hiding this comment.
Awesome! I think users will get a lot out of this and its really helpful to see this for the payload storage workstream as well.
|
|
||
| ## Claim Check Codec Implementation | ||
|
|
||
| The `ClaimCheckCodec` implements `PayloadCodec` and adds an inline threshold to keep small payloads inline for debuggability. |
There was a problem hiding this comment.
debuggability and latency presumably. Also, I might infer from this that one can't debug the payloads that you're offloading, but having read the rest of the PR, that's not what you mean.
|
|
||
| ```python | ||
| class ClaimCheckCodec(PayloadCodec): | ||
| def __init__(self, redis_host: str = "localhost", redis_port: int = 6379, max_inline_bytes: int = 20 * 1024): |
There was a problem hiding this comment.
max_inline_bytes: Say why you chose this number. Or admitting that you didn't have a great reason is valuable too.
There was a problem hiding this comment.
I think it's important that we give a reason / help people think through how they can set this number.
| *File: claim_check_plugin.py* | ||
|
|
||
| ```python | ||
| class ClaimCheckPlugin(Plugin): |
| self._next_plugin = next_plugin | ||
|
|
||
| def configure_client(self, config: ClientConfig) -> ClientConfig: | ||
| default_converter_class = config["data_converter"].payload_converter_class |
There was a problem hiding this comment.
Why is this being called the "default" in this context?
| return out | ||
|
|
||
| # Endpoint to view raw payload data | ||
| async def view_raw_data(req: web.Request) -> web.Response: |
There was a problem hiding this comment.
For the debugging-in-temporal-UI use case, I'm thinking users would want to have a JSON pretty printer when the content-type was that?
There was a problem hiding this comment.
Not sure what you mean here, within the Temporal UI we only show this summary for externally stored payloads. Do you mean when they follow the link the codec server should pretty print the raw payload if it was JSON?
There was a problem hiding this comment.
Sorry, for some reason my editor shows your comments against the wrong line number :/ I think pretty printing or otherwise should be left to the reader.
| payload_codec=claim_check_codec | ||
| ) | ||
|
|
||
| # Delegate to next plugin if it exists |
There was a problem hiding this comment.
@Quinn-With-Two-Ns check on composability
There was a problem hiding this comment.
left a comment above , will recheck if we switch to SimplePlugin
|
|
||
| config["data_converter"] = DataConverter( | ||
| payload_converter_class=default_converter_class, | ||
| payload_codec=claim_check_codec |
There was a problem hiding this comment.
If there is already a payload_codec I think you are dropping it
There was a problem hiding this comment.
Talking with Tim, it sounds like we need to add a CompositePlugin
Use minio to allow for easy local testing.
What was changed
Added example of claim check pattern.
Checklist
Closes
How was this tested:
Run locally.
Any docs updates needed?