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.

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

Add global context index and indices handler #43

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,12 @@ dependencies {

test {
include '**/*Tests.class'
systemProperty 'tests.security.manager', 'false'
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

def opensearch_tmp_dir = rootProject.file('build/private/opensearch_tmp').absoluteFile
opensearch_tmp_dir.mkdirs()

jacocoTestReport {
dependsOn test
reports {
Expand All @@ -137,6 +141,29 @@ task integTest(type: RestIntegTestTask) {
tasks.named("check").configure { dependsOn(integTest) }

integTest {
dependsOn "bundlePlugin"
systemProperty 'tests.security.manager', 'false'
systemProperty 'java.io.tmpdir', opensearch_tmp_dir.absolutePath
systemProperty "https", System.getProperty("https")
systemProperty "user", System.getProperty("user")
systemProperty "password", System.getProperty("password")

// // Only rest case can run with remote cluster
// if (System.getProperty("tests.rest.cluster") != null) {
// filter {
// includeTestsMatching "org.opensearch.flowframework.rest.*IT"
// }
// }
//
// if (System.getProperty("https") == null || System.getProperty("https") == "false") {
// filter {
// }
// }

filter {
excludeTestsMatching "org.opensearch.flowframework.indices.*Tests"
}
Copy link
Member

Choose a reason for hiding this comment

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

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?


// The --debug-jvm command-line option makes the cluster debuggable; this makes the tests debuggable
if (System.getProperty("test.debug") != null) {
jvmArgs '-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:5005'
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/
package org.opensearch.flowframework.constant;
Copy link
Member

Choose a reason for hiding this comment

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

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


/**
* Representation of common names that used across the project
*/
public class CommonName {

Check warning on line 14 in src/main/java/org/opensearch/flowframework/constant/CommonName.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L14 was not covered by tests
public static final String GLOBAL_CONTEXT_INDEX_NAME = ".opensearch-flow-framework-global-context";

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/
package org.opensearch.flowframework.constant;

/**
* Representation of common values that are used across project
*/
public class CommonValue {

Check warning on line 14 in src/main/java/org/opensearch/flowframework/constant/CommonValue.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L14 was not covered by tests

public static Integer NO_SCHEMA_VERSION = 0;

Check warning on line 16 in src/main/java/org/opensearch/flowframework/constant/CommonValue.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L16 was not covered by tests
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 = "{\n "
+ " \"dynamic\": false,\n"
+ " \"_meta\": {\n"

Check warning on line 22 in src/main/java/org/opensearch/flowframework/constant/CommonValue.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/constant/CommonValue.java#L19-L22

Added lines #L19 - L22 were not covered by tests
+ " \"schema_version\": "
+ GLOBAL_CONTEXT_INDEX_SCHEMA_VERSION
+ "\n"
+ " },\n"
+ " \"properties\": {\n"
+ " \"pipeline_id\": {\n"
Copy link
Member

@joshpalis joshpalis Sep 25, 2023

Choose a reason for hiding this comment

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

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

+ " \"type\": \"keyword\"\n"
+ " },\n"
+ " \"name\": {\n"
+ " \"type\": \"text\",\n"
+ " \"fields\": {\n"
+ " \"keyword\": {\n"
+ " \"type\": \"keyword\",\n"
+ " \"ignore_above\": 256\n"
+ " }\n"
+ " }\n"
+ " },\n"
+ " \"description\": {\n"
+ " \"type\": \"text\"\n"
+ " },\n"
+ " \"use_case\": {\n"
+ " \"type\": \"keyword\"\n"
+ " },\n"
+ " \"operations\": {\n"
+ " \"type\": \"keyword\"\n"
+ " },\n"
+ " \"version\": {\n"
+ " \"type\": \"nested\",\n"
+ " \"properties\": {\n"
+ " \"template\": {\n"
+ " \"type\": \"integer\"\n"
+ " },\n"
+ " \"compatibility\": {\n"
+ " \"type\": \"integer\"\n"
+ " }\n"
+ " }\n"
+ " },\n"
+ " \"user_inputs\": {\n"
+ " \"type\": \"nested\",\n"
+ " \"properties\": {\n"
+ " \"model_id\": {\n"
+ " \"type\": \"keyword\"\n"
+ " },\n"
+ " \"input_field\": {\n"
+ " \"type\": \"keyword\"\n"
+ " },\n"
+ " \"output_field\": {\n"
+ " \"type\": \"keyword\"\n"
+ " },\n"
+ " \"ingest_pipeline_name\": {\n"
+ " \"type\": \"keyword\"\n"
+ " },\n"
+ " \"index_name\": {\n"
+ " \"type\": \"keyword\"\n"
+ " }\n"
+ " }\n"
+ " },\n"
+ " \"workflows\": {\n"
+ " \"type\": \"text\"\n"
+ " },\n"
+ " \"responses\": {\n"
+ " \"type\": \"text\"\n"
+ " },\n"
+ " \"resources_created\": {\n"
+ " \"type\": \"text\"\n"
+ " }\n"
+ " }\n"
+ "}";
Copy link
Member

Choose a reason for hiding this comment

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

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

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/
package org.opensearch.flowframework.exception;

/**
* Representation of Flow Framework Exceptions
*/
public class FlowFrameworkException extends RuntimeException {
/**
* Constructor with error message.
*
* @param message message of the exception
*/
public FlowFrameworkException(String message) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

super(message);
}

/**
* Constructor with specified cause.
* @param cause exception cause
*/
public FlowFrameworkException(Throwable cause) {
super(cause);
}

Check warning on line 30 in src/main/java/org/opensearch/flowframework/exception/FlowFrameworkException.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/exception/FlowFrameworkException.java#L29-L30

Added lines #L29 - L30 were not covered by tests

/**
* Constructor with specified error message adn cause.
* @param message error message
* @param cause exception cause
*/
public FlowFrameworkException(String message, Throwable cause) {
super(message, cause);
}

Check warning on line 39 in src/main/java/org/opensearch/flowframework/exception/FlowFrameworkException.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/exception/FlowFrameworkException.java#L38-L39

Added lines #L38 - L39 were not covered by tests
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/
package org.opensearch.flowframework.indices;

import static org.opensearch.flowframework.constant.CommonName.GLOBAL_CONTEXT_INDEX_NAME;
import static org.opensearch.flowframework.constant.CommonValue.GLOBAL_CONTEXT_INDEX_MAPPING;
import static org.opensearch.flowframework.constant.CommonValue.GLOBAL_CONTEXT_INDEX_SCHEMA_VERSION;

/**
* An enumeration of Flow Framework indices
*/
public enum FlowFrameworkIndex {
GLOBAL_CONTEXT(GLOBAL_CONTEXT_INDEX_NAME, GLOBAL_CONTEXT_INDEX_MAPPING, GLOBAL_CONTEXT_INDEX_SCHEMA_VERSION);

Check warning on line 19 in src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndex.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndex.java#L18-L19

Added lines #L18 - L19 were not covered by tests

private final String indexName;
private final String mapping;
private final Integer version;

FlowFrameworkIndex(String name, String mapping, Integer version) {
this.indexName = name;
this.mapping = mapping;
this.version = version;
}

Check warning on line 29 in src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndex.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndex.java#L25-L29

Added lines #L25 - L29 were not covered by tests

public String getIndexName() {
return indexName;

Check warning on line 32 in src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndex.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndex.java#L32

Added line #L32 was not covered by tests
}

public String getMapping() {
return mapping;

Check warning on line 36 in src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndex.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndex.java#L36

Added line #L36 was not covered by tests
}

public Integer getVersion() {
return version;

Check warning on line 40 in src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndex.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndex.java#L40

Added line #L40 was not covered by tests
}
}
Loading