Skip to content

1713: add multi-cluster IT testing class for Cross-Cluster support #319

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

Closed
acarbonetto wants to merge 2 commits into from
Closed

1713: add multi-cluster IT testing class for Cross-Cluster support #319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0a6f6d
1713: add multi-cluster IT testing class for Cross-Cluster support
acarbonetto Aug 1, 2023
c23ed04
Add multiClusterSearch/CrossClusterSearchIT.java
acarbonetto Aug 4, 2023
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
3 changes: 2 additions & 1 deletion .github/workflows/sql-test-workflow.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,9 @@ jobs:
./gradlew :core:jacocoTestCoverageVerification || echo "* Jacoco failed for core" >> report.log
./gradlew :protocol:jacocoTestCoverageVerification || echo "* Jacoco failed for protocol" >> report.log
./gradlew :opensearch-sql-plugin:jacocoTestCoverageVerification || echo "* Jacoco failed for plugin" >> report.log
# Misc tests
# Misc/Additional Integration tests
./gradlew :integ-test:integTest || echo "* Integration test failed" >> report.log
./gradlew :integ-test:multiClusterSearch || echo "* Multi-Cluster Search tests failed" >> report.log

@GumpacG GumpacG Aug 2, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since this is for every push, can we run it after doctests? For development, doctests are more relevant to us than testing with security enabled.

@acarbonetto acarbonetto Aug 2, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I thought this was akin to Integration Testing, just against a different cluster. The tests run are integration tests.
I would categorize doctests are more API testing which seems more highlevel... but is just another form of Integration Test.
So... 🤷

@GumpacG GumpacG Aug 2, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Right it's more so when we push and we want to know if all integ test and doctest pass we would have to wait x time for mutlicluster IT to finish (spin up, run tests, and clean up) before getting our results. While the ITs added here are really only relevant for release testing.

@Yury-Fridlyand Yury-Fridlyand Aug 2, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This CI runs on demand only

@Yury-Fridlyand Yury-Fridlyand Aug 4, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If you want to run these tests in every push/PR, add it to sql-test-and-build-workflow.yml

./gradlew :doctest:doctest || echo "* Doctest failed" >> report.log
./scripts/bwctest.sh || echo "* Backward compatibility test failed" >> report.log

Expand Down
75 changes: 74 additions & 1 deletion integ-test/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,9 @@ integTest {
exclude 'org/opensearch/sql/doctest/**/*IT.class'
exclude 'org/opensearch/sql/correctness/**'

// Skip to run these IT tests on a different cluster
exclude 'org/opensearch/sql/multiClusterSearch/**'

// Explain IT is dependent on internal implementation of old engine so it's not necessary
// to run these with new engine and not necessary to make this consistent with old engine.
exclude 'org/opensearch/sql/legacy/ExplainIT.class'
Expand All @@ -301,7 +304,7 @@ integTest {
exclude 'org/opensearch/sql/jdbc/**'

// Exclude this IT until running IT with security plugin enabled is ready
exclude 'org/opensearch/sql/ppl/CrossClusterSearchIT.class'
// exclude 'org/opensearch/sql/ppl/CrossClusterSearchIT.class'

@GumpacG GumpacG Aug 2, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Delete

@Yury-Fridlyand Yury-Fridlyand Aug 4, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

☝️

}


Expand Down Expand Up @@ -481,6 +484,76 @@ task bwcTestSuite(type: StandaloneRestIntegTestTask) {
dependsOn tasks.named("${baseName}#fullRestartClusterTask")
}

testClusters {
multiClusterSearch {
testDistribution = "ARCHIVE"
numberOfNodes = 3
plugin ":opensearch-sql-plugin"
}
}

testClusters {
Comment on lines +493 to +495

@Yury-Fridlyand Yury-Fridlyand Aug 8, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
}
testClusters {

multiClusterSearchRemote {
testDistribution = "ARCHIVE"
plugin ":opensearch-sql-plugin"
}
}

task multiClusterSearch(type: RestIntegTestTask) {

useCluster testClusters.multiClusterSearch
useCluster testClusters.multiClusterSearchRemote

testLogging {
events "passed", "skipped", "failed"
}

// Set properties for connection to clusters and between clusters
doFirst {
getClusters().forEach { cluster ->
String allTransportSocketURI = cluster.nodes.stream().flatMap { node ->
node.getAllTransportPortURI().stream()
}.collect(Collectors.joining(","))
String allHttpSocketURI = cluster.nodes.stream().flatMap { node ->
node.getAllHttpSocketURI().stream()
}.collect(Collectors.joining(","))

systemProperty "tests.rest.${cluster.name}.http_hosts", "${-> allHttpSocketURI}"
systemProperty "tests.rest.${cluster.name}.transport_hosts", "${-> allTransportSocketURI}"
}
}

dependsOn ':opensearch-sql-plugin:bundlePlugin'

systemProperty 'tests.security.manager', 'false'
systemProperty('project.root', project.projectDir.absolutePath)

systemProperty "https", System.getProperty("https")
systemProperty "user", System.getProperty("user")
systemProperty "password", System.getProperty("password")
Comment on lines +531 to +533

@Yury-Fridlyand Yury-Fridlyand Aug 4, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is no-op, can be removed


// Set default query size limit
systemProperty 'defaultQuerySizeLimit', '10000'

// Tell the test JVM if the cluster JVM is running under a debugger so that tests can use longer timeouts for
// requests. The 'doFirst' delays reading the debug setting on the cluster till execution time.
doFirst {
if (System.getProperty("debug-jvm") != null) {
setDebug(true);
}
systemProperty 'cluster.debug', getDebug()
}


if (System.getProperty("test.debug") != null) {
jvmArgs '-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:5006'
}

filter {
includeTestsMatching "org.opensearch.sql.multiClusterSearch.*IT"
}
}

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

Expand Down
12 changes: 6 additions & 6 deletions integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,10 @@ protected RestClient buildClient(Settings settings, HttpHost[] hosts) throws IOE
}

// Modified from initClient in OpenSearchRestTestCase
public void initRemoteClient() throws IOException {
public void initRemoteClient(String clusterName) throws IOException {

@MitchellGale MitchellGale Aug 1, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does this not reduce potential future reuse of this function?

@acarbonetto acarbonetto Aug 2, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The cluster name is defined in the gradle task. Sean hardcoded the name in a static variable, so this only used to work with the remoteCluster, and now it works with whatever the gradle task has named the cluster.
So... no. I think this is more reusable?

if (remoteClient == null) {
assert remoteAdminClient == null;
String cluster = getTestRestCluster(REMOTE_CLUSTER);
String cluster = getTestRestCluster(clusterName);
String[] stringUrls = cluster.split(",");
List<HttpHost> hosts = new ArrayList<>(stringUrls.length);
for (String stringUrl : stringUrls) {
Expand Down Expand Up @@ -252,14 +252,14 @@ protected static void configureHttpsClient(RestClientBuilder builder, Settings s
* Initialize rest client to remote cluster,
* and create a connection to it from the coordinating cluster.
*/
public void configureMultiClusters() throws IOException {
initRemoteClient();
public void configureMultiClusters(String clusterName) throws IOException {
initRemoteClient(clusterName);

Request connectionRequest = new Request("PUT", "_cluster/settings");
String connectionSetting = "{\"persistent\": {\"cluster\": {\"remote\": {\""
+ REMOTE_CLUSTER
+ clusterName
+ "\": {\"seeds\": [\""
+ getTestTransportCluster(REMOTE_CLUSTER).split(",")[0]
+ getTestTransportCluster(clusterName).split(",")[0]
+ "\"]}}}}}";
connectionRequest.setJsonEntity(connectionSetting);
adminClient().performRequest(connectionRequest);
Expand Down
147 changes: 147 additions & 0 deletions integ-test/src/test/java/org/opensearch/sql/multiClusterSearch/CrossClusterSearchIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/


package org.opensearch.sql.multiClusterSearch;

import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT;
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK;
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DOG;
import static org.opensearch.sql.util.MatcherUtils.columnName;
import static org.opensearch.sql.util.MatcherUtils.rows;
import static org.opensearch.sql.util.MatcherUtils.verifyColumn;
import static org.opensearch.sql.util.MatcherUtils.verifyDataRows;

import java.io.IOException;
import org.json.JSONObject;
import org.junit.Rule;
import org.junit.jupiter.api.Test;
import org.junit.rules.ExpectedException;
import org.opensearch.client.ResponseException;
import org.opensearch.sql.ppl.PPLIntegTestCase;

public class CrossClusterSearchIT extends PPLIntegTestCase {

@Yury-Fridlyand Yury-Fridlyand Aug 4, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Probably add a comment describing difference with ppl/CrossClusterSearchIT


@Rule
public ExpectedException exceptionRule = ExpectedException.none();

public static final String REMOTE_CLUSTER = "multiClusterSearchRemote";

private final static String TEST_INDEX_BANK_REMOTE = REMOTE_CLUSTER + ":" + TEST_INDEX_BANK;
private final static String TEST_INDEX_DOG_REMOTE = REMOTE_CLUSTER + ":" + TEST_INDEX_DOG;
private final static String TEST_INDEX_DOG_MATCH_ALL_REMOTE = MATCH_ALL_REMOTE_CLUSTER + ":" + TEST_INDEX_DOG;
private final static String TEST_INDEX_ACCOUNT_REMOTE = REMOTE_CLUSTER + ":" + TEST_INDEX_ACCOUNT;

@Override
public void init() throws IOException {
configureMultiClusters(REMOTE_CLUSTER);
loadIndex(Index.BANK);
loadIndex(Index.BANK, remoteClient());
loadIndex(Index.DOG);
loadIndex(Index.DOG, remoteClient());
loadIndex(Index.ACCOUNT, remoteClient());
}

@Test
public void testCrossClusterSearchAllFields() throws IOException {
JSONObject result = executeQuery(String.format("search source=%s", TEST_INDEX_DOG_REMOTE));
verifyColumn(result, columnName("dog_name"), columnName("holdersName"), columnName("age"));
}

@Test
public void testMatchAllCrossClusterSearchAllFields() throws IOException {
JSONObject result = executeQuery(String.format("search source=%s", TEST_INDEX_DOG_MATCH_ALL_REMOTE));
verifyColumn(result, columnName("dog_name"), columnName("holdersName"), columnName("age"));
}

@Test
public void testCrossClusterSearchWithoutLocalFieldMappingShouldFail() throws IOException {
exceptionRule.expect(ResponseException.class);
exceptionRule.expectMessage("400 Bad Request");
exceptionRule.expectMessage("IndexNotFoundException");

executeQuery(String.format("search source=%s", TEST_INDEX_ACCOUNT_REMOTE));
}

@Test
public void testCrossClusterSearchCommandWithLogicalExpression() throws IOException {
JSONObject result = executeQuery(String.format(
"search source=%s firstname='Hattie' | fields firstname", TEST_INDEX_BANK_REMOTE));
verifyDataRows(result, rows("Hattie"));
}

@Test
public void testCrossClusterSearchMultiClusters() throws IOException {
JSONObject result = executeQuery(String.format(
"search source=%s,%s firstname='Hattie' | fields firstname", TEST_INDEX_BANK_REMOTE, TEST_INDEX_BANK));
verifyDataRows(result,
rows("Hattie"),
rows("Hattie"));
}

@Test
public void testCrossClusterDescribeAllFields() throws IOException {
JSONObject result = executeQuery(String.format("describe %s", TEST_INDEX_DOG_REMOTE));
verifyColumn(
result,
columnName("TABLE_CAT"),
columnName("TABLE_SCHEM"),
columnName("TABLE_NAME"),
columnName("COLUMN_NAME"),
columnName("DATA_TYPE"),
columnName("TYPE_NAME"),
columnName("COLUMN_SIZE"),
columnName("BUFFER_LENGTH"),
columnName("DECIMAL_DIGITS"),
columnName("NUM_PREC_RADIX"),
columnName("NULLABLE"),
columnName("REMARKS"),
columnName("COLUMN_DEF"),
columnName("SQL_DATA_TYPE"),
columnName("SQL_DATETIME_SUB"),
columnName("CHAR_OCTET_LENGTH"),
columnName("ORDINAL_POSITION"),
columnName("IS_NULLABLE"),
columnName("SCOPE_CATALOG"),
columnName("SCOPE_SCHEMA"),
columnName("SCOPE_TABLE"),
columnName("SOURCE_DATA_TYPE"),
columnName("IS_AUTOINCREMENT"),
columnName("IS_GENERATEDCOLUMN")
);
}

@Test
public void testMatchAllCrossClusterDescribeAllFields() throws IOException {
JSONObject result = executeQuery(String.format("describe %s", TEST_INDEX_DOG_MATCH_ALL_REMOTE));
verifyColumn(
result,
columnName("TABLE_CAT"),
columnName("TABLE_SCHEM"),
columnName("TABLE_NAME"),
columnName("COLUMN_NAME"),
columnName("DATA_TYPE"),
columnName("TYPE_NAME"),
columnName("COLUMN_SIZE"),
columnName("BUFFER_LENGTH"),
columnName("DECIMAL_DIGITS"),
columnName("NUM_PREC_RADIX"),
columnName("NULLABLE"),
columnName("REMARKS"),
columnName("COLUMN_DEF"),
columnName("SQL_DATA_TYPE"),
columnName("SQL_DATETIME_SUB"),
columnName("CHAR_OCTET_LENGTH"),
columnName("ORDINAL_POSITION"),
columnName("IS_NULLABLE"),
columnName("SCOPE_CATALOG"),
columnName("SCOPE_SCHEMA"),
columnName("SCOPE_TABLE"),
columnName("SOURCE_DATA_TYPE"),
columnName("IS_AUTOINCREMENT"),
columnName("IS_GENERATEDCOLUMN")
);
}
}
2 changes: 1 addition & 1 deletion integ-test/src/test/java/org/opensearch/sql/ppl/CrossClusterSearchIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public class CrossClusterSearchIT extends PPLIntegTestCase {

@Override
public void init() throws IOException {
configureMultiClusters();
configureMultiClusters(REMOTE_CLUSTER);
loadIndex(Index.BANK);
loadIndex(Index.BANK, remoteClient());
loadIndex(Index.DOG);
Expand Down