You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by GitBox <gi...@apache.org> on 2022/04/29 05:22:34 UTC

[GitHub] [jackrabbit-oak] nit0906 opened a new pull request, #554: Using a global ES docker container that is only created once durign t…

nit0906 opened a new pull request, #554:
URL: https://github.com/apache/jackrabbit-oak/pull/554

   …he execution of oak-search-elastic


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [jackrabbit-oak] fabriziofortino commented on a diff in pull request #554: Using a global ES docker container that is only created once durign t…

Posted by GitBox <gi...@apache.org>.
fabriziofortino commented on code in PR #554:
URL: https://github.com/apache/jackrabbit-oak/pull/554#discussion_r861520536


##########
oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticConnectionRule.java:
##########
@@ -17,12 +17,15 @@
 package org.apache.jackrabbit.oak.plugins.index.elastic;
 
 import com.github.dockerjava.api.DockerClient;
+import org.apache.commons.lang3.RandomStringUtils;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClients;
 import org.apache.jackrabbit.oak.commons.IOUtils;
 import org.elasticsearch.Version;
+import org.elasticsearch.action.admin.indices.delete.DeleteIndexRequest;
+import org.elasticsearch.client.RequestOptions;
 import org.junit.rules.ExternalResource;
 import org.junit.runner.Description;
 import org.junit.runners.model.Statement;

Review Comment:
   cleanup the unused imports



##########
oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticConnectionRule.java:
##########
@@ -70,62 +73,22 @@ public ElasticConnectionRule(String elasticConnectionString) {
     @Override
     public Statement apply(Statement base, Description description) {
         Statement s = super.apply(base, description);
-        // see if docker is to be used or not... initialize docker rule only if that's the case.
-        final String pluginVersion = "7.16.3.0";
-        final String pluginFileName = "elastiknn-" + pluginVersion + ".zip";
-        final String localPluginPath = "target/" + pluginFileName;
-        downloadSimilaritySearchPluginIfNotExists(localPluginPath, pluginVersion);
         if (elasticConnectionString == null || getElasticConnectionFromString() == null) {
-            checkIfDockerClientAvailable();
-            Network network = Network.newNetwork();
-
-            elastic = new ElasticsearchContainer("docker.elastic.co/elasticsearch/elasticsearch:" + Version.CURRENT)
-                    .withCopyFileToContainer(MountableFile.forHostPath(localPluginPath), "/tmp/plugins/" + pluginFileName)
-                    .withCopyFileToContainer(MountableFile.forClasspathResource("elasticstartscript.sh"), "/tmp/elasticstartscript.sh")
-                    .withCommand("bash /tmp/elasticstartscript.sh")
-                    .withNetwork(network);
-            elastic.start();
-
+            elastic = ElasticTestServer.getESTestServer().container();
             setUseDocker(true);
         }
         return s;
     }
 
     @Override
     protected void after() {
-        if (elastic != null && elastic.isRunning()) {
-            elastic.stop();
-        }
-    }
-
-    private void downloadSimilaritySearchPluginIfNotExists(String localPluginPath, String pluginVersion) {
-        File pluginFile = new File(localPluginPath);
-        if (!pluginFile.exists()) {
-            LOG.info("Plugin file {} doesn't exist. Trying to download.", localPluginPath);
-            try (CloseableHttpClient client = HttpClients.createDefault()) {
-                HttpGet get = new HttpGet("https://github.com/alexklibisz/elastiknn/releases/download/" + pluginVersion
-                        + "/elastiknn-" + pluginVersion + ".zip");
-                CloseableHttpResponse response = client.execute(get);
-                InputStream inputStream = response.getEntity().getContent();
-                MessageDigest messageDigest = MessageDigest.getInstance("SHA-256");
-                DigestInputStream dis = new DigestInputStream(inputStream, messageDigest);
-                FileOutputStream outputStream = new FileOutputStream(pluginFile);
-                IOUtils.copy(dis, outputStream);
-                messageDigest = dis.getMessageDigest();
-                // bytes to hex
-                StringBuilder result = new StringBuilder();
-                for (byte b : messageDigest.digest()) {
-                    result.append(String.format("%02x", b));
-                }
-                if (!PLUGIN_DIGEST.equals(result.toString())) {
-                    String deleteString = "Downloaded plugin file deleted.";
-                    if (!pluginFile.delete()) {
-                        deleteString = "Could not delete downloaded plugin file.";
-                    }
-                    throw new RuntimeException("Plugin digest unequal. Found " + result + ". Expected " + PLUGIN_DIGEST + ". " + deleteString);
-                }
-            } catch (IOException | NoSuchAlgorithmException e) {
-                throw new RuntimeException("Could not download similarity search plugin", e);
+        ElasticConnection esConnection = useDocker() ? getElasticConnectionForDocker() : getElasticConnectionFromString();
+        if (esConnection != null) {
+            try {
+                esConnection.getClient().indices().delete(new DeleteIndexRequest(esConnection.getIndexPrefix() + "*"), RequestOptions.DEFAULT);
+                esConnection.close();
+            } catch (IOException e) {
+                LOG.error("Unable to delete indexes with prefix {}", esConnection.getIndexPrefix());

Review Comment:
   we have a `tearDown` method in `ElasticAbstractQueryTest` that does pretty much the same. We should remove the delete indices from there



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [jackrabbit-oak] nit0906 commented on a diff in pull request #554: Using a global ES docker container that is only created once durign t…

Posted by GitBox <gi...@apache.org>.
nit0906 commented on code in PR #554:
URL: https://github.com/apache/jackrabbit-oak/pull/554#discussion_r861573061


##########
oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticConnectionRule.java:
##########
@@ -17,12 +17,15 @@
 package org.apache.jackrabbit.oak.plugins.index.elastic;
 
 import com.github.dockerjava.api.DockerClient;
+import org.apache.commons.lang3.RandomStringUtils;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClients;
 import org.apache.jackrabbit.oak.commons.IOUtils;
 import org.elasticsearch.Version;
+import org.elasticsearch.action.admin.indices.delete.DeleteIndexRequest;
+import org.elasticsearch.client.RequestOptions;
 import org.junit.rules.ExternalResource;
 import org.junit.runner.Description;
 import org.junit.runners.model.Statement;

Review Comment:
   done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [jackrabbit-oak] nit0906 commented on a diff in pull request #554: Using a global ES docker container that is only created once durign t…

Posted by GitBox <gi...@apache.org>.
nit0906 commented on code in PR #554:
URL: https://github.com/apache/jackrabbit-oak/pull/554#discussion_r861536624


##########
oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticConnectionRule.java:
##########
@@ -70,62 +73,22 @@ public ElasticConnectionRule(String elasticConnectionString) {
     @Override
     public Statement apply(Statement base, Description description) {
         Statement s = super.apply(base, description);
-        // see if docker is to be used or not... initialize docker rule only if that's the case.
-        final String pluginVersion = "7.16.3.0";
-        final String pluginFileName = "elastiknn-" + pluginVersion + ".zip";
-        final String localPluginPath = "target/" + pluginFileName;
-        downloadSimilaritySearchPluginIfNotExists(localPluginPath, pluginVersion);
         if (elasticConnectionString == null || getElasticConnectionFromString() == null) {
-            checkIfDockerClientAvailable();
-            Network network = Network.newNetwork();
-
-            elastic = new ElasticsearchContainer("docker.elastic.co/elasticsearch/elasticsearch:" + Version.CURRENT)
-                    .withCopyFileToContainer(MountableFile.forHostPath(localPluginPath), "/tmp/plugins/" + pluginFileName)
-                    .withCopyFileToContainer(MountableFile.forClasspathResource("elasticstartscript.sh"), "/tmp/elasticstartscript.sh")
-                    .withCommand("bash /tmp/elasticstartscript.sh")
-                    .withNetwork(network);
-            elastic.start();
-
+            elastic = ElasticTestServer.getESTestServer().container();
             setUseDocker(true);
         }
         return s;
     }
 
     @Override
     protected void after() {
-        if (elastic != null && elastic.isRunning()) {
-            elastic.stop();
-        }
-    }
-
-    private void downloadSimilaritySearchPluginIfNotExists(String localPluginPath, String pluginVersion) {
-        File pluginFile = new File(localPluginPath);
-        if (!pluginFile.exists()) {
-            LOG.info("Plugin file {} doesn't exist. Trying to download.", localPluginPath);
-            try (CloseableHttpClient client = HttpClients.createDefault()) {
-                HttpGet get = new HttpGet("https://github.com/alexklibisz/elastiknn/releases/download/" + pluginVersion
-                        + "/elastiknn-" + pluginVersion + ".zip");
-                CloseableHttpResponse response = client.execute(get);
-                InputStream inputStream = response.getEntity().getContent();
-                MessageDigest messageDigest = MessageDigest.getInstance("SHA-256");
-                DigestInputStream dis = new DigestInputStream(inputStream, messageDigest);
-                FileOutputStream outputStream = new FileOutputStream(pluginFile);
-                IOUtils.copy(dis, outputStream);
-                messageDigest = dis.getMessageDigest();
-                // bytes to hex
-                StringBuilder result = new StringBuilder();
-                for (byte b : messageDigest.digest()) {
-                    result.append(String.format("%02x", b));
-                }
-                if (!PLUGIN_DIGEST.equals(result.toString())) {
-                    String deleteString = "Downloaded plugin file deleted.";
-                    if (!pluginFile.delete()) {
-                        deleteString = "Could not delete downloaded plugin file.";
-                    }
-                    throw new RuntimeException("Plugin digest unequal. Found " + result + ". Expected " + PLUGIN_DIGEST + ". " + deleteString);
-                }
-            } catch (IOException | NoSuchAlgorithmException e) {
-                throw new RuntimeException("Could not download similarity search plugin", e);
+        ElasticConnection esConnection = useDocker() ? getElasticConnectionForDocker() : getElasticConnectionFromString();
+        if (esConnection != null) {
+            try {
+                esConnection.getClient().indices().delete(new DeleteIndexRequest(esConnection.getIndexPrefix() + "*"), RequestOptions.DEFAULT);
+                esConnection.close();
+            } catch (IOException e) {
+                LOG.error("Unable to delete indexes with prefix {}", esConnection.getIndexPrefix());

Review Comment:
   For those cases (the ones that extend ElasticAbstractQueryTest), the delete call in ElasticConnectionRule would just be one extra unwanted call (since it would effectively be NOOP)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [jackrabbit-oak] nit0906 merged pull request #554: Using a global ES docker container that is only created once durign t…

Posted by GitBox <gi...@apache.org>.
nit0906 merged PR #554:
URL: https://github.com/apache/jackrabbit-oak/pull/554


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [jackrabbit-oak] nit0906 commented on a diff in pull request #554: Using a global ES docker container that is only created once durign t…

Posted by GitBox <gi...@apache.org>.
nit0906 commented on code in PR #554:
URL: https://github.com/apache/jackrabbit-oak/pull/554#discussion_r861535555


##########
oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticConnectionRule.java:
##########
@@ -70,62 +73,22 @@ public ElasticConnectionRule(String elasticConnectionString) {
     @Override
     public Statement apply(Statement base, Description description) {
         Statement s = super.apply(base, description);
-        // see if docker is to be used or not... initialize docker rule only if that's the case.
-        final String pluginVersion = "7.16.3.0";
-        final String pluginFileName = "elastiknn-" + pluginVersion + ".zip";
-        final String localPluginPath = "target/" + pluginFileName;
-        downloadSimilaritySearchPluginIfNotExists(localPluginPath, pluginVersion);
         if (elasticConnectionString == null || getElasticConnectionFromString() == null) {
-            checkIfDockerClientAvailable();
-            Network network = Network.newNetwork();
-
-            elastic = new ElasticsearchContainer("docker.elastic.co/elasticsearch/elasticsearch:" + Version.CURRENT)
-                    .withCopyFileToContainer(MountableFile.forHostPath(localPluginPath), "/tmp/plugins/" + pluginFileName)
-                    .withCopyFileToContainer(MountableFile.forClasspathResource("elasticstartscript.sh"), "/tmp/elasticstartscript.sh")
-                    .withCommand("bash /tmp/elasticstartscript.sh")
-                    .withNetwork(network);
-            elastic.start();
-
+            elastic = ElasticTestServer.getESTestServer().container();
             setUseDocker(true);
         }
         return s;
     }
 
     @Override
     protected void after() {
-        if (elastic != null && elastic.isRunning()) {
-            elastic.stop();
-        }
-    }
-
-    private void downloadSimilaritySearchPluginIfNotExists(String localPluginPath, String pluginVersion) {
-        File pluginFile = new File(localPluginPath);
-        if (!pluginFile.exists()) {
-            LOG.info("Plugin file {} doesn't exist. Trying to download.", localPluginPath);
-            try (CloseableHttpClient client = HttpClients.createDefault()) {
-                HttpGet get = new HttpGet("https://github.com/alexklibisz/elastiknn/releases/download/" + pluginVersion
-                        + "/elastiknn-" + pluginVersion + ".zip");
-                CloseableHttpResponse response = client.execute(get);
-                InputStream inputStream = response.getEntity().getContent();
-                MessageDigest messageDigest = MessageDigest.getInstance("SHA-256");
-                DigestInputStream dis = new DigestInputStream(inputStream, messageDigest);
-                FileOutputStream outputStream = new FileOutputStream(pluginFile);
-                IOUtils.copy(dis, outputStream);
-                messageDigest = dis.getMessageDigest();
-                // bytes to hex
-                StringBuilder result = new StringBuilder();
-                for (byte b : messageDigest.digest()) {
-                    result.append(String.format("%02x", b));
-                }
-                if (!PLUGIN_DIGEST.equals(result.toString())) {
-                    String deleteString = "Downloaded plugin file deleted.";
-                    if (!pluginFile.delete()) {
-                        deleteString = "Could not delete downloaded plugin file.";
-                    }
-                    throw new RuntimeException("Plugin digest unequal. Found " + result + ". Expected " + PLUGIN_DIGEST + ". " + deleteString);
-                }
-            } catch (IOException | NoSuchAlgorithmException e) {
-                throw new RuntimeException("Could not download similarity search plugin", e);
+        ElasticConnection esConnection = useDocker() ? getElasticConnectionForDocker() : getElasticConnectionFromString();
+        if (esConnection != null) {
+            try {
+                esConnection.getClient().indices().delete(new DeleteIndexRequest(esConnection.getIndexPrefix() + "*"), RequestOptions.DEFAULT);
+                esConnection.close();
+            } catch (IOException e) {
+                LOG.error("Unable to delete indexes with prefix {}", esConnection.getIndexPrefix());

Review Comment:
   @fabriziofortino  - the tearDown there runs after every method (indexes are created before every method and then cleaned up after it's executed), which is needed for a few cases there. 
   
   But unfortunately not all tests extend that, that's why I added one in the ElasticConnectionRule.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org