-
Notifications
You must be signed in to change notification settings - Fork 0
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
update apis for commands definitions #15
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments
@@ -258,11 +258,9 @@ components: | |||
TimerCommand: | |||
type: object | |||
required: | |||
- firingUnixTimestampSeconds | |||
- delayInSeconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like it.
We used firingUnixTimestampSeconds
since iWF -- but the absolute time value makes it a little hard for write unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. The client should just send a delay and the server will transform the delay to an abosolute timestamp.
api-schema/xdb.yaml
Outdated
type: array | ||
items: | ||
$ref: '#/components/schemas/LocalQueueMessageResult' | ||
LocalQueueStatus: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably LocalQueueCommandStatus
or just QueueCommandStatus
is more accurate. QueueStatus could mean something else for representing the status of the whole queue.
I am also open to combine this with TimerStatus to CommandStatus
:
- WAITING_COMMAND
- COMPLETED_COMMAND
- SKIPPED_COMMAND
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combining is a good idea to me.
api-schema/xdb.yaml
Outdated
enum: | ||
- WAITING_LOCAL_QUEUE | ||
- COMPLETED_LOCAL_QUEUE | ||
- SKIPPED_LOCAL_QUEUE # in anyOfCompletion, set this status for other waiting local queues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure it's really a good idea to set it SKIPPED
for that case. For timer status, SKIPPED
is specific for the case where a timer is skipped intentionally by an API request : https://github.com/indeedeng/iwf-idl/blob/main/iwf.yaml#L236
When a timer is skipped, it is equivalent as "fired" for the waitUntil. This is for mitigate some stuck timers or fire a timer earlier for testing/operation purpose.
So when a queue command is not completed, may be just WAITING is fine to be consistent with the semantics.
Also let’s improve the pr title . It will become the commit message of main branch |
This reverts commit dc97b72.
This reverts commit 24ba119.
No description provided.