Add global context index and indices handler #43

jackiehanyang · 2023-09-18T17:03:12Z

Description

Creating an indices handler to create index while if it's absent, or upgrade the mapping if it's needed.
Adding use-case template index mapping
Adding a class to throw our own custom exceptions.

Integration tests will be added later when the setup for integration test CI is ready.

Issues Resolved

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.

Signed-off-by: Jackie Han <[email protected]>

dbwiddis

Initial pass

dbwiddis · 2023-09-18T18:31:01Z

src/main/java/org/opensearch/flowframework/constant/CommonValue.java

+    public static final String META = "_meta";
+    public static final String SCHEMA_VERSION_FIELD = "schema_version";
+    public static final Integer GLOBAL_CONTEXT_INDEX_SCHEMA_VERSION = 1;
+    public static final String GLOBAL_CONTEXT_INDEX_MAPPING =


Can we put this in src/main/resources and read it into this constant off the class path? See here.

I'm fine with putting this index mapping in either path; I believe it's just a matter of coding style. However, I do prefer to put it in under src/main/java cause it can save us from setting up methods to read from src/main/resources, something like getMapping() ML-Commons is using this approach as well, see here

dbwiddis · 2023-09-18T18:36:07Z

build.gradle

@@ -105,6 +105,7 @@ repositories {
 dependencies {
    implementation "org.opensearch:opensearch:${opensearch_version}"
    implementation 'org.junit.jupiter:junit-jupiter:5.10.0'
+    implementation 'org.projectlombok:lombok:1.18.22'


We should have a separate discussion/issue/RFC on whether we want to use a code generation tool like Lombok, and figure out what modifications we need to make to our CI to support it.

removed lombok dependency for now. It's not needed in this pr. I will open a discuss and figure out the XContent question before adding this dependency

dbwiddis · 2023-09-18T18:39:11Z

src/main/java/org/opensearch/flowframework/exception/FlowFrameworkException.java

+     *
+     * @param message message of the exception
+     */
+    public FlowFrameworkException(String message) {


I like the idea of having our own custom exceptions. Given that these will eventually need to be processed in a RestResponse, can we consider including the appropriate return code as part of constructing this exception? For example, "index not found" should return RestStatus.NOT_FOUND etc.

Signed-off-by: Jackie Han <[email protected]>

codecov · 2023-09-25T16:36:54Z

Codecov Report

Merging #43 (bce1b62) into main (1d22bee) will decrease coverage by 22.17%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##               main      #43       +/-   ##
=============================================
- Coverage     73.87%   51.71%   -22.17%     
  Complexity       65       65               
=============================================
  Files             8       13        +5     
  Lines           245      350      +105     
  Branches         22       29        +7     
=============================================
  Hits            181      181               
- Misses           54      159      +105     
  Partials         10       10

Files	Coverage Δ
.../opensearch/flowframework/constant/CommonName.java	`0.00% <0.00%> (ø)`
...opensearch/flowframework/constant/CommonValue.java	`0.00% <0.00%> (ø)`
...lowframework/exception/FlowFrameworkException.java	`0.00% <0.00%> (ø)`
...arch/flowframework/indices/FlowFrameworkIndex.java	`0.00% <0.00%> (ø)`
...framework/indices/FlowFrameworkIndicesHandler.java	`0.00% <0.00%> (ø)`

joshpalis

Initial pass

joshpalis · 2023-09-25T17:43:34Z

build.gradle

@@ -117,8 +117,12 @@ dependencies {

 test {
    include '**/*Tests.class'
+    systemProperty 'tests.security.manager', 'false'


Trying to understand these build.gradle settings better, does this disable security for tests?

We should not disable security manager for the plugin. I know it's deprecated but OpenSearch still uses it and doesn't have our security manager yet.

joshpalis · 2023-09-25T17:44:46Z

src/main/java/org/opensearch/flowframework/constant/CommonValue.java

+        + "\n"
+        + "   },\n"
+        + "   \"properties\": {\n"
+        + "       \"pipeline_id\": {\n"


let's change this pipeline_id to workflow_id to avoid confusion with ingest/search pipelines

joshpalis · 2023-09-25T17:45:51Z

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java

+public class FlowFrameworkIndicesHandler {
+
+    private static final Logger logger = LogManager.getLogger(FlowFrameworkIndicesHandler.class);
+    private static final Map<String, Object> indexSettings = Map.of("index.auto_expand_replicas", "0-1");


What's the justification for this auto_expand_replicas setting, just trying to understand why this is necessary

joshpalis · 2023-09-25T17:48:55Z

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java

+        String indexName = index.getIndexName();
+        String mapping = index.getMapping();
+
+        try (ThreadContext.StoredContext threadContext = client.threadPool().getThreadContext().stashContext()) {


From what I understand about security, we only need to stash the thread context if this particular index is managed by the security plugin's list of protected indices. This allows the plugin to do CRUD operations on this protected index. Is the plan to add all flow framework indices to the security plugin's list of protected indices, and if not, why stash the thread context?

joshpalis · 2023-09-25T17:49:43Z

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java

+                CreateIndexRequest request = new CreateIndexRequest(indexName).mapping(mapping).settings(indexSettings);
+                client.admin().indices().create(request, actionListener);
+            } else {
+                logger.debug("index:{} is already created", indexName);


I'd prefer that we keep logging at the info or error level for success and failure cases respectively

I personally don't mind extra debug or even trace logging.

owaiskazi19

Need tests! Since we both kind of worked on creating the index. I will create a new PR to have the generic way of creating the index and later you can pass the Global Context mapping file to it to create GC index.

owaiskazi19 · 2023-09-25T20:57:18Z

build.gradle

@@ -117,8 +117,12 @@ dependencies {

 test {
    include '**/*Tests.class'
+    systemProperty 'tests.security.manager', 'false'


We should not disable security manager for the plugin. I know it's deprecated but OpenSearch still uses it and doesn't have our security manager yet.

owaiskazi19 · 2023-09-25T20:58:23Z

build.gradle

+
+    filter {
+        excludeTestsMatching "org.opensearch.flowframework.indices.*Tests"
+    }


Can we remove the integ test code from this PR and add it in the next PR as this PR doesn't supports the framework?

owaiskazi19 · 2023-09-25T21:24:47Z

src/main/java/org/opensearch/flowframework/constant/CommonValue.java

+        + "           \"type\": \"text\"\n"
+        + "       }\n"
+        + "   }\n"
+        + "}";


We should have a JSON file for mapping and then parse it to create a String.

owaiskazi19 · 2023-09-25T22:00:54Z

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java

+        Map<String, Object> indexMapping = indexMetaData.mapping().getSourceAsMap();
+        Object meta = indexMapping.get(META);
+        if (meta != null && meta instanceof Map) {
+            @SuppressWarnings("unchecked")


Why do we have suppress warnings here?

I suspect because all we know is that meta is an Object that an instance of Map but we don't know the types of its key and value when they are cast.

dbwiddis · 2023-09-26T05:29:35Z

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java

+        Map<String, Object> indexMapping = indexMetaData.mapping().getSourceAsMap();
+        Object meta = indexMapping.get(META);
+        if (meta != null && meta instanceof Map) {
+            @SuppressWarnings("unchecked")


I suspect because all we know is that meta is an Object that an instance of Map but we don't know the types of its key and value when they are cast.

dbwiddis · 2023-09-26T05:32:01Z

src/main/java/org/opensearch/flowframework/constant/CommonName.java

+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+package org.opensearch.flowframework.constant;


Curious about this choice of package name. Given the class names, seems common might be better?

dbwiddis · 2023-09-26T05:37:53Z

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java

+                    logger.error("Failed to create index " + indexName, e);
+                    internalListener.onFailure(e);
+                });
+                CreateIndexRequest request = new CreateIndexRequest(indexName).mapping(mapping).settings(indexSettings);


does the mapping here require a mediatype? Looking at the javadoc it seems it expects JSON but needs a special format with a _doc key. We shold be consistent between our various implementations so all our mapping files look consistent.

dbwiddis · 2023-09-26T05:38:27Z

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java

+                CreateIndexRequest request = new CreateIndexRequest(indexName).mapping(mapping).settings(indexSettings);
+                client.admin().indices().create(request, actionListener);
+            } else {
+                logger.debug("index:{} is already created", indexName);


I personally don't mind extra debug or even trace logging.

dbwiddis · 2023-09-26T05:41:27Z

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java

+                            client.admin()
+                                .indices()
+                                .putMapping(
+                                    new PutMappingRequest().indices(indexName).source(mapping, XContentType.JSON),


I think this requires a MediaType argument instead of XContentType.

Add global context index and indices handler

1096170

Signed-off-by: Jackie Han <[email protected]>

github-actions bot added the backport 2.x backport PRs to 2.x branch label Sep 18, 2023

dbwiddis reviewed Sep 18, 2023

View reviewed changes

jackiehanyang added 2 commits September 21, 2023 09:59

Update global context index mapping

14b9a95

Signed-off-by: Jackie Han <[email protected]>

correct checkstyle errors

2069fb2

Signed-off-by: Jackie Han <[email protected]>

dbwiddis mentioned this pull request Sep 22, 2023

Adds Create Index Building block #38

Merged

jackiehanyang added 2 commits September 22, 2023 15:12

skip index handler integ tests

a4d052e

Signed-off-by: Jackie Han <[email protected]>

remove indices integration tests for now

21c0788

Signed-off-by: Jackie Han <[email protected]>

jackiehanyang marked this pull request as ready for review September 25, 2023 16:50

jackiehanyang requested review from owaiskazi19, joshpalis, ohltyler and amitgalitz as code owners September 25, 2023 16:50

joshpalis reviewed Sep 25, 2023

View reviewed changes

owaiskazi19 requested changes Sep 25, 2023

View reviewed changes

Merge branch 'opensearch-project:main' into main

bce1b62

dbwiddis reviewed Sep 26, 2023

View reviewed changes

jackiehanyang closed this Sep 28, 2023

dbwiddis mentioned this pull request Oct 1, 2023

Introduce global-context index and related operations #65

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add global context index and indices handler #43

Add global context index and indices handler #43

jackiehanyang commented Sep 18, 2023 •

edited

Loading

dbwiddis left a comment

dbwiddis Sep 18, 2023

jackiehanyang Sep 21, 2023 •

edited

Loading

dbwiddis Sep 18, 2023

jackiehanyang Sep 21, 2023

dbwiddis Sep 18, 2023

codecov bot commented Sep 25, 2023 •

edited

Loading

joshpalis left a comment

joshpalis Sep 25, 2023

owaiskazi19 Sep 25, 2023

joshpalis Sep 25, 2023 •

edited

Loading

joshpalis Sep 25, 2023

joshpalis Sep 25, 2023

joshpalis Sep 25, 2023

dbwiddis Sep 26, 2023

owaiskazi19 left a comment

owaiskazi19 Sep 25, 2023

owaiskazi19 Sep 25, 2023

owaiskazi19 Sep 25, 2023

owaiskazi19 Sep 25, 2023

dbwiddis Sep 26, 2023

dbwiddis Sep 26, 2023

dbwiddis Sep 26, 2023

dbwiddis Sep 26, 2023

dbwiddis Sep 26, 2023

dbwiddis Sep 26, 2023

Add global context index and indices handler #43

Add global context index and indices handler #43

Conversation

jackiehanyang commented Sep 18, 2023 • edited Loading

Description

Issues Resolved

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackiehanyang Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 25, 2023 • edited Loading

Codecov Report

joshpalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshpalis Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owaiskazi19 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackiehanyang commented Sep 18, 2023 •

edited

Loading

jackiehanyang Sep 21, 2023 •

edited

Loading

codecov bot commented Sep 25, 2023 •

edited

Loading

joshpalis Sep 25, 2023 •

edited

Loading