search

manetu / temporal-clojure-sdk

A Temporal SDK for Clojure
Apache License 2.0
72 stars 10 forks source link

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

Closed muralisrini closed 6 months ago

muralisrini commented 6 months ago

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.

muralisrini commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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.