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.

Sign up for GitHub

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

Jump to bottom

Replace all CompletableFutures with PlainActionFutures #419

Merged
merged 8 commits into from
Jan 22, 2024
Merged

Replace all CompletableFutures with PlainActionFutures #419

merged 8 commits into from
Jan 22, 2024

Conversation

owaiskazi19
Copy link
Member

@owaiskazi19 owaiskazi19 commented Jan 18, 2024 •
edited
Loading

Description

Replace all CompletableFutures with PlainActionFutures
Except

  • CompletableFuture.runAsync

Issues Resolved

Fixes #409

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.

@github-actions github-actions bot added the backport 2.x backport PRs to 2.x branch label Jan 18, 2024
@owaiskazi19
Copy link
Member Author

@zane-neo mind taking a look.

@codecov Codecov
Copy link

codecov bot commented Jan 18, 2024 •
edited
Loading

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (42e4cd4) 71.87% compared to head (1c660aa) 71.88%.

Files Patch % Lines
...ch/flowframework/workflow/CreateConnectorStep.java 50.00% 3 Missing ⚠️
...search/flowframework/workflow/CreateIndexStep.java 50.00% 3 Missing ⚠️
...owframework/workflow/CreateIngestPipelineStep.java 57.14% 3 Missing ⚠️
...arch/flowframework/workflow/RegisterAgentStep.java 50.00% 3 Missing ⚠️
...lowframework/workflow/RegisterRemoteModelStep.java 57.14% 3 Missing ⚠️
...rk/transport/ProvisionWorkflowTransportAction.java 0.00% 2 Missing ⚠️
...ework/workflow/AbstractRegisterLocalModelStep.java 71.42% 2 Missing ⚠️
...flowframework/workflow/RegisterModelGroupStep.java 66.66% 2 Missing ⚠️
...search/flowframework/workflow/DeployModelStep.java 80.00% 1 Missing ⚠️
...rg/opensearch/flowframework/workflow/ToolStep.java 66.66% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##               main     #419   +/-   ##
=========================================
  Coverage     71.87%   71.88%           
- Complexity      619      620    +1     
=========================================
  Files            78       78           
  Lines          3133     3126    -7     
  Branches        238      236    -2     
=========================================
- Hits           2252     2247    -5     
+ Misses          774      772    -2     
  Partials        107      107

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dbwiddis
dbwiddis reviewed Jan 18, 2024
View reviewed changes
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM! A few suggestions:

  • We use the completed future for WorkflowData.EMPTY so often we should make it a constant (probalby in the WorkflowData class
  • You can really streamline the ProcessNode collection of predecessor future values. The previous multi-step was only there to enable a varargs array for allOf().
  • Replace all future.isDone() with a test for future.getState() matching COMPLETED.
  • Double-check the exception handling everywhere you've replaced join() with either get() or actionGet() as it's different.

src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/flowframework/transport/DeprovisionWorkflowTransportAction.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/flowframework/workflow/CreateIngestPipelineStep.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/flowframework/workflow/DeleteModelGroupStep.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/flowframework/workflow/NoOpStep.java Show resolved Hide resolved
src/test/java/org/opensearch/flowframework/workflow/DeleteAgentStepTests.java Show resolved Hide resolved
src/test/java/org/opensearch/flowframework/workflow/DeleteAgentStepTests.java Show resolved Hide resolved
src/test/java/org/opensearch/flowframework/workflow/DeleteConnectorStepTests.java Show resolved Hide resolved
src/test/java/org/opensearch/flowframework/workflow/DeleteConnectorStepTests.java Show resolved Hide resolved
src/test/java/org/opensearch/flowframework/workflow/ProcessNodeTests.java Show resolved Hide resolved
@dbwiddis dbwiddis changed the title [Draft] Intial commit to remove CompletableFuture Replace all CompletableFutures with PlainActionFutures Jan 18, 2024
@owaiskazi19 owaiskazi19 marked this pull request as ready for review January 20, 2024 00:57
dbwiddis
dbwiddis approved these changes Jan 20, 2024
View reviewed changes
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM!

.github/workflows/test_security.yml Outdated Show resolved Hide resolved
src/test/java/org/opensearch/flowframework/workflow/CreateConnectorStepTests.java Show resolved Hide resolved
@dbwiddis
Move log line
107b495 
Signed-off-by: Daniel Widdis <[email protected]>
@dbwiddis
Reenable multi-node tests
f505cba 
Signed-off-by: Daniel Widdis <[email protected]>
@dbwiddis
Disable fail-fast
1c660aa 
Signed-off-by: Daniel Widdis <[email protected]>
joshpalis
joshpalis approved these changes Jan 22, 2024
View reviewed changes
Copy link
Member

@joshpalis joshpalis left a comment

Choose a reason for hiding this comment

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

Changes LGTM

@owaiskazi19 owaiskazi19 merged commit 92d9108 into opensearch-project:main Jan 22, 2024
25 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 22, 2024
@github-actions @dbwiddis
Replace all CompletableFutures with PlainActionFutures (#419)
f06b525 
* Intial commit to remove CompletableFuture

Signed-off-by: Owais Kazi <[email protected]>

* Removed CompletableFuture from ProcessNode and tests

Signed-off-by: Owais Kazi <[email protected]>

* Removed CompletableFuture from create index and pipeline workflow steps

Signed-off-by: Owais Kazi <[email protected]>

* Passing tests

Signed-off-by: Owais Kazi <[email protected]>

* Addressed initial comments

Signed-off-by: Owais Kazi <[email protected]>

* Move log line

Signed-off-by: Daniel Widdis <[email protected]>

* Reenable multi-node tests

Signed-off-by: Daniel Widdis <[email protected]>

* Disable fail-fast

Signed-off-by: Daniel Widdis <[email protected]>

---------

Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Co-authored-by: Daniel Widdis <[email protected]>
(cherry picked from commit 92d9108)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jan 22, 2024
Merged
dbwiddis added a commit that referenced this pull request Jan 22, 2024
@opensearch-trigger-bot @github-actions @dbwiddis
[Backport 2.x] Replace all CompletableFutures with PlainActionFutures (…
dbfaef8 
…#433)

Replace all CompletableFutures with PlainActionFutures (#419)

* Intial commit to remove CompletableFuture



* Removed CompletableFuture from ProcessNode and tests



* Removed CompletableFuture from create index and pipeline workflow steps



* Passing tests



* Addressed initial comments



* Move log line



* Reenable multi-node tests



* Disable fail-fast



---------




(cherry picked from commit 92d9108)

Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: Daniel Widdis <[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>
Co-authored-by: Daniel Widdis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zane-neo zane-neo zane-neo left review comments

@dbwiddis dbwiddis dbwiddis approved these changes

@joshpalis joshpalis joshpalis approved these changes

@ohltyler ohltyler Awaiting requested review from ohltyler ohltyler is a code owner

@amitgalitz amitgalitz Awaiting requested review from amitgalitz amitgalitz is a code owner

@jackiehanyang jackiehanyang Awaiting requested review from jackiehanyang jackiehanyang is a code owner

Assignees
No one assigned
Labels
backport 2.x backport PRs to 2.x branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Multi Node failures
4 participants
@owaiskazi19 @dbwiddis @joshpalis @zane-neo