-
Notifications
You must be signed in to change notification settings - Fork 37
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 synchronous execution option to workflow provisioning #990
base: main
Are you sure you want to change the base?
Add synchronous execution option to workflow provisioning #990
Conversation
Signed-off-by: Junwei Dai <[email protected]>
Signed-off-by: Junwei Dai <[email protected]>
Signed-off-by: Junwei Dai <[email protected]>
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.
Generally looks good.
- You need to handle -1 time value; my recommendation is you use that for the default "async" rather than null
- You need to do stream version checks for the new (optional) workflow state in the response, and the new timeout parameter in the workflow request (unless you want to just keep it in the params map).
@@ -87,6 +89,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli | |||
boolean provision = request.paramAsBoolean(PROVISION_WORKFLOW, false); | |||
boolean reprovision = request.paramAsBoolean(REPROVISION_WORKFLOW, false); | |||
boolean updateFields = request.paramAsBoolean(UPDATE_WORKFLOW_FIELDS, false); | |||
TimeValue waitForCompletionTimeout = request.paramAsTime(WAIT_FOR_COMPLETION_TIMEOUT, null); |
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 generally like to avoid null
whenever possible. It can crop up in unexpected places (toString()
gets called somewhere, for example).
There is a special constant TimeValue.MINUS_ONE
you can use to "disable" the feature. Then later rather than testing != null
you can just test for this special value.
This is a common pattern in OpenSearch when using TimeValue
settings or parameters, so it is behavior that existing users should expect. See for example cancel_after_time_interval.
In fact, since -1 is an allowed timevalue, unless you explicitly test for it to not be negative, you'll allow that particular edge case (which will probably behave similarly to 0, returning immediately?).
No other negative values are allowed in timevalues, so you can reliably use a <0 test for this.
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.
That said, I'm not sure we need to parse it here, rather than just passing it along in the params
map to be handled in the transport action?
src/main/java/org/opensearch/flowframework/transport/WorkflowRequest.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/WorkflowResponse.java
Show resolved
Hide resolved
src/test/java/org/opensearch/flowframework/rest/RestCreateWorkflowActionTests.java
Show resolved
Hide resolved
) { | ||
threadPool.executor(PROVISION_WORKFLOW_THREAD_POOL).execute(() -> { | ||
try { | ||
Thread.sleep(timeout); |
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.
There are probably better ways to handle timeouts than sleeping threads.
Async search uses a TimeoutRunnableListener<Response>
to execute a block of code when the timeout expires. We should probably follow that pattern.
Description
This PR introduces a new
wait_for_completion_timeout
feature to the Provision Workflow API in the OpenSearch Flow Framework. The feature allows users to control whether the API call waits for the entire workflow provisioning process to complete before returning a response.What’s Changed:
Success Response:
TimeOut Response:
Areas of Concern:
I have a few parts of the implementation that I believe can be further improved, particularly in ProvisionWorkflowTransportAction. Some of the logic feels a bit verbose and might not be the most efficient way to handle the timeout and synchronous execution. I’d appreciate the feedback from reviewers.
Related Issues
Resolves #967
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.