-
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
Adding a state index #110
Adding a state index #110
Conversation
9469bf1
to
cd9ddb9
Compare
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.
Initial pass, looks good so far. I had a few comments regarding exception handling, but I'll just rebase PR #106 after this is merged in and handle it there.
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.
Initially LGTM with some nits.
I can't get this to build locally, there's a version mismatch in log4j. Would like this rebased so I can compile before approving.
Dependency resolution failed because of conflict on the following module:
- org.apache.logging.log4j:log4j-api between versions 2.21.0 and 2.20.0
org.apache.logging.log4j:log4j-api:2.21.0
Variant compile:
| Attribute Name | Provided | Requested |
|--------------------------------|----------|--------------|
| org.gradle.status | release | |
| org.gradle.category | library | library |
| org.gradle.libraryelements | jar | classes |
| org.gradle.usage | java-api | java-api |
| org.gradle.dependency.bundling | | external |
| org.gradle.jvm.environment | | standard-jvm |
| org.gradle.jvm.version | | 11 |
Selection reasons:
- By conflict resolution: between versions 2.21.0 and 2.20.0
org.apache.logging.log4j:log4j-api:2.21.0
\--- compileClasspath
org.apache.logging.log4j:log4j-api:2.20.0 -> 2.21.0
+--- org.opensearch:opensearch:3.0.0-SNAPSHOT
| \--- compileClasspath
\--- org.opensearch:opensearch-core:3.0.0-SNAPSHOT
+--- org.opensearch:opensearch:3.0.0-SNAPSHOT (*)
+--- org.opensearch:opensearch-compress:3.0.0-SNAPSHOT
| \--- org.opensearch:opensearch:3.0.0-SNAPSHOT (*)
\--- org.opensearch:opensearch-x-content:3.0.0-SNAPSHOT
\--- org.opensearch:opensearch:3.0.0-SNAPSHOT (*)
src/main/java/org/opensearch/flowframework/util/ParseUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/workflow/CreateIndexStep.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandlerTests.java
Outdated
Show resolved
Hide resolved
yes I am still fixing some commit conflicts from latest PR that was merged in. |
cd9ddb9
to
818405e
Compare
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 progress!
src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/util/ParseUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/workflow/CreateIndexStep.java
Show resolved
Hide resolved
src/test/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandlerTests.java
Outdated
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.
Approved with a small comment, thanks @amitgalitz for implementing this. One ask that I have is if you could post the WorkflowState
document created when invoking the CreateWorkflow
rest action and when invoking the ProvisionWorkflow
rest action.
Signed-off-by: Amit Galitzky <[email protected]>
Signed-off-by: Amit Galitzky <[email protected]>
Signed-off-by: Amit Galitzky <[email protected]>
2f8efed
to
730d61e
Compare
Codecov Report
@@ Coverage Diff @@
## main #110 +/- ##
=============================================
- Coverage 80.24% 66.05% -14.20%
- Complexity 250 266 +16
=============================================
Files 30 34 +4
Lines 992 1299 +307
Branches 95 123 +28
=============================================
+ Hits 796 858 +62
- Misses 160 399 +239
- Partials 36 42 +6
|
src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.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.
Almost there. With few open comments. Let's answer/resolve them and get this in
Signed-off-by: Amit Galitzky <[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.
Thanks for addressing the comments! Approved. Let's address the remaining comments/tests in a follow up PR.
* adding state index initial Signed-off-by: Amit Galitzky <[email protected]> * addressed comments and added more fields to state index Signed-off-by: Amit Galitzky <[email protected]> * addressed comments and fixed some unit tests Signed-off-by: Amit Galitzky <[email protected]> * moved variables to common value and adressed other comments Signed-off-by: Amit Galitzky <[email protected]> --------- Signed-off-by: Amit Galitzky <[email protected]> (cherry picked from commit 4106cba) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Adding a state index (#110) * adding state index initial * addressed comments and added more fields to state index * addressed comments and fixed some unit tests * moved variables to common value and adressed other comments --------- (cherry picked from commit 4106cba) Signed-off-by: Amit Galitzky <[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
Adding a state index along with new methods to add documents to the new state index and update the state index.
Logs when we run the create API:
Issues Resolved
#74
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.