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

Add send_local_queue_message and rename workerTask to ImmediateTask #9

Merged
merged 6 commits into from
Oct 21, 2023

Conversation

zklgame
Copy link
Contributor

@zklgame zklgame commented Oct 20, 2023

No description provided.

@zklgame zklgame changed the title Add send-local-message Add send_local_queue_message and rename workerTask to ImmediateTask Oct 20, 2023
@zklgame zklgame marked this pull request as ready for review October 20, 2023 07:29
@zklgame zklgame changed the title Add send_local_queue_message and rename workerTask to ImmediateTask Add send_local_queue_message Oct 20, 2023
@zklgame zklgame changed the title Add send_local_queue_message Add send_local_queue_message and rename workerTask to ImmediateTask Oct 20, 2023
@@ -84,7 +84,31 @@ paths:
application/json:
schema:
$ref: '#/components/schemas/ApiErrorResponse'
# xdb workers APIs are hosted by iWF applications via SDKs, for iWF server to call back
/api/v1/xdb/service/process-execution/send-message-to-local-queue:
Copy link
Contributor

Choose a reason for hiding this comment

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

As we said the name is too long, I have been thinking the name. Actually in iwf, it's called publish_to_internal_channel. Maybe we can also call it publish_to_local_queue here? which one do you think is "slightly" better?

Copy link
Contributor

Choose a reason for hiding this comment

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

and potentially in the future, the API can publish multiple messages at the same time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

publish-to-local-queue sounds good to me!

@@ -521,4 +583,23 @@ components:
enum:
- TERMINATE # default behaviour. ProcessStatus becomes TERMINATED
- FAIL # ProcessStatus becomes FAILED
ProcessExecutionPublishToLocalQueueRequest:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just PublishToLocalQueueRequest? I feel like that's clear enough as it has local in the name

type: string
queueName:
type: string
dedupId:
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, maybe we can just let it publish an array from beginning. It doesn't seem like a lot of additional work to implement. And doing it later will require a lot of refactoring and backward compatibility work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea.

Copy link
Contributor

@longquanzheng longquanzheng left a comment

Choose a reason for hiding this comment

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

In case you forget, we actually allow publishing messages at waitUntil/execute APIs. This is an mechanism for multiple states communication. You can add it separately, but I feel like it's easier to add it in the same api PR, and implement them increamentally in server/sdk.

https://github.com/indeedeng/iwf-idl/blob/af1891eb032c534b0b75894c611ead7e2954777d/iwf-sdk.yaml#L1066

Copy link
Contributor

@longquanzheng longquanzheng left a comment

Choose a reason for hiding this comment

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

lgtm cc @duoertai as well as he will be working on timer command later which will need to do something similar

@zklgame zklgame merged commit dc8027d into main Oct 21, 2023
1 check passed
@zklgame zklgame deleted the add_send_local_message branch October 21, 2023 07:59
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.

None yet

2 participants