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.

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

schedule's args serdes should adhere to this SDKs protocol #55

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

muralisrini
Copy link
Contributor

Args typically are serialized and deserialized using objarray-> and args-> (defined in temporal.internal.utils) so that data-conversion can transparently work with arbitrary data. Schedule should adhere to this protocol and apply objarray-> on arguments so as to satisfy the args-> call when workflow is executed.

TODO: enhance UT to actually launch a workflow when base java library adds proper schedule support to its "testing" framework.

Args typically are serialized and deserialized using objarray-> and
args-> (defined in temporal.internal.utils) so that data-conversion can
transparently work with arbitrary data.  Schedule should adhere to this
protocol and apply objarray-> on arguments so as to satisfy the args->
call when workflow is executed.

TODO: enhance UT to actually launch a workflow when base java library
adds proper schedule support to its "testing" framework.

Signed-off-by: Srinivasan Muralidharan <smuralidharan@manetu.com>
@muralisrini muralisrini changed the title args serdes should adhere to this SDKs protocol schedule's args serdes should adhere to this SDKs protocol Apr 29, 2024
@muralisrini muralisrini changed the title schedule's args serdes should adhere to this SDKs protocol Draft: schedule's args serdes should adhere to this SDKs protocol Apr 29, 2024
@muralisrini
Copy link
Contributor Author

@mintybayleaf first of all thank you for the contribution. Came handy just in time for me.

One thing though... I followed the doc https://cljdoc.org/d/io.github.manetu/temporal-sdk/0.20.1/doc/clients?q=schedule#scheduled-workflow-executions but got an exception when the workflow was launched. The workflow launched by temporal scheduler was trying to deserialize arguments using ->args (around line 74 in internal/workflow.clj) and raised an exception as the serialization was not done using ->objarray. This PR attempts to use ->objarrary for serialization as the symmetrical use of ->objarray and ->args for serdes seems to be the pattern.

However I wonder if I'm missing something and may not be using the schedule api as intended. Can you take a peek and give feedback please? In particular, how are you using it?

@mintybayleaf
Copy link
Contributor

mintybayleaf commented Apr 29, 2024

@mintybayleaf first of all thank you for the contribution. Came handy just in time for me.

One thing though... I followed the doc https://cljdoc.org/d/io.github.manetu/temporal-sdk/0.20.1/doc/clients?q=schedule#scheduled-workflow-executions but got an exception when the workflow was launched. The workflow launched by temporal scheduler was trying to deserialize arguments using ->args (around line 74 in internal/workflow.clj) and raised an exception as the serialization was not done using ->objarray. This PR attempts to use ->objarrary for serialization as the symmetrical use of ->objarray and ->args for serdes seems to be the pattern.

However I wonder if I'm missing something and may not be using the schedule api as intended. Can you take a peek and give feedback please? In particular, how are you using it?

@muralisrini

Thank you for using the code! I think this might have been an oversight on my part when I copied a bulk of the code over.

The symmetry here makes sense, and I just simply missed it because we wrote this code internally and did not use the internal ->objarray function at that time. I pulled down your change and tested it with our scheduled workflows.

To answer your actual question we are converting the bytes -> json and vice versa so we could use the cli and the ui

So good catch! This seems to be backwards compatible as well.

@muralisrini
Copy link
Contributor Author

@mintybayleaf first of all thank you for the contribution. Came handy just in time for me.
One thing though... I followed the doc https://cljdoc.org/d/io.github.manetu/temporal-sdk/0.20.1/doc/clients?q=schedule#scheduled-workflow-executions but got an exception when the workflow was launched. The workflow launched by temporal scheduler was trying to deserialize arguments using ->args (around line 74 in internal/workflow.clj) and raised an exception as the serialization was not done using ->objarray. This PR attempts to use ->objarrary for serialization as the symmetrical use of ->objarray and ->args for serdes seems to be the pattern.
However I wonder if I'm missing something and may not be using the schedule api as intended. Can you take a peek and give feedback please? In particular, how are you using it?

@muralisrini

Thank you for using the code! I think this might have been an oversight on my part when I copied a bulk of the code over.

The symmetry here makes sense, and I just simply missed it because we wrote this code internally and did not use the internal ->objarray function at that time. I pulled down your change and tested it with our scheduled workflows.

To answer your actual question we are converting the bytes -> json and vice versa so we could use the cli and the ui

So good catch! This seems to be backwards compatible as well.

perfect, ty for the quick response!

I've removed Draft now.

@muralisrini muralisrini changed the title Draft: schedule's args serdes should adhere to this SDKs protocol schedule's args serdes should adhere to this SDKs protocol Apr 29, 2024
@ghaskins ghaskins merged commit b7c3965 into master Apr 30, 2024
2 checks passed
@ghaskins ghaskins deleted the make_args_serdes_symmetrical branch April 30, 2024 11:24
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