-
Notifications
You must be signed in to change notification settings - Fork 36
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
Updates exception handling with FlowFrameworkException AND adds dryrun param to Create Workflow #137
Conversation
…user outputs Signed-off-by: Joshua Palis <[email protected]>
…s into previous_node_inputs and user_inputs, adds graph validation after topologically sorting a workflow into a list of ProcessNode Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
…async, adding success test case for graph validation Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
…ass and using constants Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
…ctions to create flow framework exceptions Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #137 +/- ##
============================================
- Coverage 69.05% 67.09% -1.97%
- Complexity 302 311 +9
============================================
Files 37 37
Lines 1393 1468 +75
Branches 132 139 +7
============================================
+ Hits 962 985 +23
- Misses 386 434 +48
- Partials 45 49 +4
|
Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java
Show resolved
Hide resolved
…ed on exceptions thrown by the transport client Signed-off-by: Joshua Palis <[email protected]>
src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java
Show resolved
Hide resolved
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.
Looks good overall. General comment. Concatenate the error message with e.getMessage() in FlowFrameworkException.
…ore saving Signed-off-by: Joshua Palis <[email protected]>
I've actioned #99 as well in this PR. Example create workflow request with a cyclical graph
|
src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java
Show resolved
Hide resolved
Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[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.
LGTM! One suggestion is to keep the PR shorter and making it easy for review.
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.
Great work!
We're pretty inconsistent with our error messages. Not sure if we're too far gone to do anything about it though!
src/main/java/org/opensearch/flowframework/exception/FlowFrameworkException.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Joshua Palis <[email protected]>
…n param to Create Workflow (#137) * Simplifying Template format, removing operations, resources created, user outputs Signed-off-by: Joshua Palis <[email protected]> * Initial commit, modifies use case template to seperate workflow inputs into previous_node_inputs and user_inputs, adds graph validation after topologically sorting a workflow into a list of ProcessNode Signed-off-by: Joshua Palis <[email protected]> * Adding tests Signed-off-by: Joshua Palis <[email protected]> * Adding validate graph test Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments, moving sorting/validating prior to executing async, adding success test case for graph validation Signed-off-by: Joshua Palis <[email protected]> * Adding javadocs Signed-off-by: Joshua Palis <[email protected]> * Moving validation prior to updating workflow state to provisioning Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments Part 1 Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments Part 2 : Moving field names to common value class and using constants Signed-off-by: Joshua Palis <[email protected]> * Adding definition for noop workflow step Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments Part 3 Signed-off-by: Joshua Palis <[email protected]> * Modifies rest actions to throw flow framework exceptions, transport actions to create flow framework exceptions Signed-off-by: Joshua Palis <[email protected]> * Fixing credentials field in workflow-step json Signed-off-by: Joshua Palis <[email protected]> * Fixing test Signed-off-by: Joshua Palis <[email protected]> * Using ExceptionsHelper.status() to determine the rest status code based on exceptions thrown by the transport client Signed-off-by: Joshua Palis <[email protected]> * Adding dryrun param to create workflow API, allows for validation before saving Signed-off-by: Joshua Palis <[email protected]> * concatenating log message with exception message on failure Signed-off-by: Joshua Palis <[email protected]> * Adding dry run test Signed-off-by: Joshua Palis <[email protected]> * Simplifying FlowFrameworkException::toXContent Signed-off-by: Joshua Palis <[email protected]> --------- Signed-off-by: Joshua Palis <[email protected]> (cherry picked from commit c547658) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
… AND adds dryrun param to Create Workflow (#144) Updates exception handling with FlowFrameworkException AND adds dryrun param to Create Workflow (#137) * Simplifying Template format, removing operations, resources created, user outputs * Initial commit, modifies use case template to seperate workflow inputs into previous_node_inputs and user_inputs, adds graph validation after topologically sorting a workflow into a list of ProcessNode * Adding tests * Adding validate graph test * Addressing PR comments, moving sorting/validating prior to executing async, adding success test case for graph validation * Adding javadocs * Moving validation prior to updating workflow state to provisioning * Addressing PR comments Part 1 * Addressing PR comments Part 2 : Moving field names to common value class and using constants * Adding definition for noop workflow step * Addressing PR comments Part 3 * Modifies rest actions to throw flow framework exceptions, transport actions to create flow framework exceptions * Fixing credentials field in workflow-step json * Fixing test * Using ExceptionsHelper.status() to determine the rest status code based on exceptions thrown by the transport client * Adding dryrun param to create workflow API, allows for validation before saving * concatenating log message with exception message on failure * Adding dry run test * Simplifying FlowFrameworkException::toXContent --------- (cherry picked from commit c547658) Signed-off-by: Joshua Palis <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
This PR updates the workflow parsing, workflow validation, workflow execution and rest action exception handling to throw
FlowFrameworkExceptions
with the correct REST Status code.Note : I have opted not to throw
FlowFrameworkExceptions
for any of the workflow steps, since these steps are executed after we return a rest response back to the user.FlowFrameworkExceptions
allow us to control the status of a rest response, since workflow step exceptions will be indexed into theuser_outputs
field of theState
index, there's no need for a rest status for those casesExample Create workflow request with missing
create_connector_step
:Attempt to provision will fail due to graph validation, response is returned in the correct format with the correct rest status :
OpenSearch logs that graph validation failed :
Issues Resolved
Part of #88
#99
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.