-
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
Adds transport request retry capability for GetMLTaskStep #179
Conversation
…skStep Signed-off-by: Joshua Palis <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
============================================
+ Coverage 72.12% 72.24% +0.12%
- Complexity 407 413 +6
============================================
Files 53 54 +1
Lines 1948 1971 +23
Branches 164 165 +1
============================================
+ Hits 1405 1424 +19
- Misses 471 474 +3
- Partials 72 73 +1 ☔ View full report in Codecov by Sentry. |
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.
Great to see the retry functionality! Much needed. Few comments regarding the implementation.
src/main/java/org/opensearch/flowframework/FlowFrameworkPlugin.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/workflow/GetMLTaskStep.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/common/FlowFrameworkMaxRequestRetrySetting.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/common/FlowFrameworkSettings.java
Outdated
Show resolved
Hide resolved
… to initialize the setting update consumer for max retry request setting, passing settings and clusterservice to retryable workflow step instead Signed-off-by: Joshua Palis <[email protected]>
src/main/java/org/opensearch/flowframework/workflow/RetryableWorkflowStep.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Joshua Palis <[email protected]>
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! Keeping the comment unresolved for Dan to answer. I'm assuming retry will be tested with integ tests?
src/main/java/org/opensearch/flowframework/common/FlowFrameworkSettings.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/common/FlowFrameworkSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/common/FlowFrameworkSettings.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/common/FlowFrameworkSettings.java
Outdated
Show resolved
Hide resolved
…oving max value from setting, cancelling future on thread interuppt 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!
* Added FlowFrameworkMaxRequestRetrySetting and applied this to GetMlTaskStep Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments, creating abstract class RetryableWorkflowStep to initialize the setting update consumer for max retry request setting, passing settings and clusterservice to retryable workflow step instead Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments Signed-off-by: Joshua Palis <[email protected]> * Removing retry utils class Signed-off-by: Joshua Palis <[email protected]> * Fixing failure unit tests to ensure thread sleep isnt invoked Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments, changing setting name, improving javadoc, removing max value from setting, cancelling future on thread interuppt Signed-off-by: Joshua Palis <[email protected]> --------- Signed-off-by: Joshua Palis <[email protected]> (cherry picked from commit 1092325) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Added FlowFrameworkMaxRequestRetrySetting and applied this to GetMlTaskStep Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments, creating abstract class RetryableWorkflowStep to initialize the setting update consumer for max retry request setting, passing settings and clusterservice to retryable workflow step instead Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments Signed-off-by: Joshua Palis <[email protected]> * Removing retry utils class Signed-off-by: Joshua Palis <[email protected]> * Fixing failure unit tests to ensure thread sleep isnt invoked Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments, changing setting name, improving javadoc, removing max value from setting, cancelling future on thread interuppt Signed-off-by: Joshua Palis <[email protected]> --------- Signed-off-by: Joshua Palis <[email protected]> (cherry picked from commit 1092325) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…ility for GetMLTaskStep (#205) Adds transport request retry capability for GetMLTaskStep (#179) * Added FlowFrameworkMaxRequestRetrySetting and applied this to GetMlTaskStep * Addressing PR comments, creating abstract class RetryableWorkflowStep to initialize the setting update consumer for max retry request setting, passing settings and clusterservice to retryable workflow step instead * Addressing PR comments * Removing retry utils class * Fixing failure unit tests to ensure thread sleep isnt invoked * Addressing PR comments, changing setting name, improving javadoc, removing max value from setting, cancelling future on thread interuppt --------- (cherry picked from commit 1092325) 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
Adds an updateable
FlowFrameworkMaxRequestRetry
setting to control the number of request retries for workflow steps that invoke an async API.In this case for a local model registration workflow, the
GetMLTaskStep
will first attempt to retrieve the status of the given task. If the task is notCompleted
, then the step will wait before retrying the request until the status isCompleted
. Only then will themodel_id
be availableSteps to test :
GetMLTaskStep
includes an increasednode_timeout
due to the length of time it takes to register a local modelIssues Resolved
Actioning issue : #158
Part of #88
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.