cpoerschke commented on a change in pull request #736:
URL: https://github.com/apache/solr/pull/736#discussion_r824944652
##########
File path:
solr/core/src/test/org/apache/solr/filestore/TestDistribPackageStore.java
##########
@@ -290,6 +299,9 @@ public static NavigableObject assertResponseValues(
Utils.toJSONString(actual));
}
}
+ if (passed) {
+ return rsp;
Review comment:
minor/subjective
```suggestion
break;
```
##########
File path: solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java
##########
@@ -69,214 +68,168 @@
private Phaser phaser;
@Before
- public void setup() {
+ public void setup() throws Exception {
System.setProperty("enable.packages", "true");
phaser = new Phaser();
+
+ int nodes = TEST_NIGHTLY ? 4 : 2;
+ cluster = configureCluster(nodes).withJettyConfig(jetty ->
jetty.enableV2(true)).configure();
+
cluster.getOpenOverseer().getCoreContainer().getContainerPluginsRegistry().setPhaser(phaser);
}
@After
- public void teardown() {
+ public void teardown() throws Exception {
+ shutdownCluster();
System.clearProperty("enable.packages");
}
@Test
public void testApi() throws Exception {
- MiniSolrCloudCluster cluster =
- configureCluster(4).withJettyConfig(jetty ->
jetty.enableV2(true)).configure();
- ContainerPluginsRegistry pluginsRegistry =
-
cluster.getOpenOverseer().getCoreContainer().getContainerPluginsRegistry();
- pluginsRegistry.setPhaser(phaser);
-
int version = phaser.getPhase();
- String errPath = "/error/details[0]/errorMessages[0]";
- try {
- PluginMeta plugin = new PluginMeta();
+ PluginMeta plugin = new PluginMeta();
+ V2Request addPlugin =
+ new V2Request.Builder("/cluster/plugin")
+ .forceV2(true)
+ .POST()
+ .withPayload(singletonMap("add", plugin))
+ .build();
+
+ // test with an invalid class
+ try (ErrorLogMuter errors =
ErrorLogMuter.substring("TestContainerPlugin$C2")) {
plugin.name = "testplugin";
plugin.klass = C2.class.getName();
- // test with an invalid class
- V2Request req =
- new V2Request.Builder("/cluster/plugin")
- .forceV2(true)
- .POST()
- .withPayload(singletonMap("add", plugin))
- .build();
- expectError(req, cluster.getSolrClient(), errPath, "No method with
@Command in class");
-
- // test with a valid class. This should succeed now
- plugin.klass = C3.class.getName();
- req.process(cluster.getSolrClient());
-
- version = phaser.awaitAdvanceInterruptibly(version, 10,
TimeUnit.SECONDS);
-
- // just check if the plugin is indeed registered
- V2Request readPluginState =
- new V2Request.Builder("/cluster/plugin").forceV2(true).GET().build();
- V2Response rsp = readPluginState.process(cluster.getSolrClient());
- assertEquals(C3.class.getName(), rsp._getStr("/plugin/testplugin/class",
null));
-
- // let's test the plugin
- TestDistribPackageStore.assertResponseValues(
- 10,
- () ->
- new V2Request.Builder("/plugin/my/plugin")
- .forceV2(true)
- .GET()
- .build()
- .process(cluster.getSolrClient()),
- ImmutableMap.of("/testkey", "testval"));
-
- // now remove the plugin
- new V2Request.Builder("/cluster/plugin")
- .POST()
- .forceV2(true)
- .withPayload("{remove : testplugin}")
- .build()
- .process(cluster.getSolrClient());
-
- version = phaser.awaitAdvanceInterruptibly(version, 10,
TimeUnit.SECONDS);
-
- // verify it is removed
- rsp = readPluginState.process(cluster.getSolrClient());
- assertEquals(null, rsp._get("/plugin/testplugin/class", null));
+ expectError(addPlugin, "No method with @Command in class");
+ assertEquals(1, errors.getCount());
+ }
+
+ // test with a valid class. This should succeed now
+ plugin.klass = C3.class.getName();
+ addPlugin.process(cluster.getSolrClient());
+
+ version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+ // just check if the plugin is indeed registered
+ Callable<V2Response> readPluginState = getPlugin("/cluster/plugin");
+ V2Response rsp = readPluginState.call();
+ assertEquals(C3.class.getName(), rsp._getStr("/plugin/testplugin/class",
null));
+
+ // let's test the plugin
+ TestDistribPackageStore.assertResponseValues(
+ getPlugin("/plugin/my/plugin"), Map.of("/testkey", "testval"));
+
+ // now remove the plugin
+ new V2Request.Builder("/cluster/plugin")
+ .POST()
+ .forceV2(true)
+ .withPayload("{remove : testplugin}")
+ .build()
+ .process(cluster.getSolrClient());
+
+ version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+
+ // verify it is removed
+ rsp = readPluginState.call();
+ assertNull(rsp._get("/plugin/testplugin/class", null));
+
+ try (ErrorLogMuter errors =
ErrorLogMuter.substring("TestContainerPlugin$C4")) {
// test with a class @EndPoint methods. This also uses a template in
the path name
plugin.klass = C4.class.getName();
plugin.name = "collections";
plugin.pathPrefix = "collections";
- expectError(
- req, cluster.getSolrClient(), errPath, "path must not have a prefix:
collections");
-
- plugin.name = "my-random-name";
- plugin.pathPrefix = "my-random-prefix";
-
- req.process(cluster.getSolrClient());
- version = phaser.awaitAdvanceInterruptibly(version, 10,
TimeUnit.SECONDS);
-
- // let's test the plugin
- TestDistribPackageStore.assertResponseValues(
- 10,
- () ->
- new V2Request.Builder("/my-random-name/my/plugin")
- .forceV2(true)
- .GET()
- .build()
- .process(cluster.getSolrClient()),
- ImmutableMap.of("/method.name", "m1"));
-
- TestDistribPackageStore.assertResponseValues(
- 10,
- () ->
- new V2Request.Builder("/my-random-prefix/their/plugin")
- .forceV2(true)
- .GET()
- .build()
- .process(cluster.getSolrClient()),
- ImmutableMap.of("/method.name", "m2"));
- // now remove the plugin
- new V2Request.Builder("/cluster/plugin")
- .POST()
- .forceV2(true)
- .withPayload("{remove : my-random-name}")
- .build()
- .process(cluster.getSolrClient());
-
- version = phaser.awaitAdvanceInterruptibly(version, 10,
TimeUnit.SECONDS);
-
- expectFail(
- () ->
- new V2Request.Builder("/my-random-prefix/their/plugin")
- .forceV2(true)
- .GET()
- .build()
- .process(cluster.getSolrClient()));
- expectFail(
- () ->
- new V2Request.Builder("/my-random-prefix/their/plugin")
- .forceV2(true)
- .GET()
- .build()
- .process(cluster.getSolrClient()));
-
- // test ClusterSingleton plugin
- plugin.name = "clusterSingleton";
- plugin.klass = C6.class.getName();
- req.process(cluster.getSolrClient());
- version = phaser.awaitAdvanceInterruptibly(version, 10,
TimeUnit.SECONDS);
-
- // just check if the plugin is indeed registered
- readPluginState = new
V2Request.Builder("/cluster/plugin").forceV2(true).GET().build();
- rsp = readPluginState.process(cluster.getSolrClient());
- assertEquals(C6.class.getName(),
rsp._getStr("/plugin/clusterSingleton/class", null));
-
- assertTrue("ccProvided", C6.ccProvided);
- assertTrue("startCalled", C6.startCalled);
- assertFalse("stopCalled", C6.stopCalled);
-
- assertEquals(CConfig.class, ContainerPluginsRegistry.getConfigClass(new
CC()));
- assertEquals(CConfig.class, ContainerPluginsRegistry.getConfigClass(new
CC1()));
- assertEquals(CConfig.class, ContainerPluginsRegistry.getConfigClass(new
CC2()));
-
- CConfig cfg = new CConfig();
- cfg.boolVal = Boolean.TRUE;
- cfg.strVal = "Something";
- cfg.longVal = 1234L;
- PluginMeta p = new PluginMeta();
- p.name = "hello";
- p.klass = CC.class.getName();
- p.config = cfg;
-
- new V2Request.Builder("/cluster/plugin")
- .forceV2(true)
- .POST()
- .withPayload(singletonMap("add", p))
- .build()
- .process(cluster.getSolrClient());
-
- version = phaser.awaitAdvanceInterruptibly(version, 10,
TimeUnit.SECONDS);
-
- TestDistribPackageStore.assertResponseValues(
- 10,
- () ->
- new V2Request.Builder("hello/plugin")
- .forceV2(true)
- .GET()
- .build()
- .process(cluster.getSolrClient()),
- ImmutableMap.of(
- "/config/boolVal", "true", "/config/strVal", "Something",
"/config/longVal", "1234"));
-
- cfg.strVal = "Something else";
- new V2Request.Builder("/cluster/plugin")
- .forceV2(true)
- .POST()
- .withPayload(singletonMap("update", p))
- .build()
- .process(cluster.getSolrClient());
- version = phaser.awaitAdvanceInterruptibly(version, 10,
TimeUnit.SECONDS);
-
- TestDistribPackageStore.assertResponseValues(
- 10,
- () ->
- new V2Request.Builder("hello/plugin")
- .forceV2(true)
- .GET()
- .build()
- .process(cluster.getSolrClient()),
- ImmutableMap.of(
- "/config/boolVal", "true", "/config/strVal", cfg.strVal,
"/config/longVal", "1234"));
-
- // kill the Overseer leader
- for (JettySolrRunner jetty : cluster.getJettySolrRunners()) {
- if
(!jetty.getCoreContainer().getZkController().getOverseer().isClosed()) {
- cluster.stopJettySolrRunner(jetty);
- cluster.waitForJettyToStop(jetty);
- }
+ expectError(addPlugin, "path must not have a prefix: collections");
+ assertEquals(1, errors.getCount());
+ }
+
+ plugin.name = "my-random-name";
+ plugin.pathPrefix = "my-random-prefix";
+
+ addPlugin.process(cluster.getSolrClient());
+ version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+
+ // let's test the plugin
+ TestDistribPackageStore.assertResponseValues(
+ getPlugin("/my-random-name/my/plugin"), Map.of("/method.name", "m1"));
+
+ TestDistribPackageStore.assertResponseValues(
+ getPlugin("/my-random-prefix/their/plugin"), Map.of("/method.name",
"m2"));
+ // now remove the plugin
+ new V2Request.Builder("/cluster/plugin")
+ .POST()
+ .forceV2(true)
+ .withPayload("{remove : my-random-name}")
+ .build()
+ .process(cluster.getSolrClient());
+
+ version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+
+ try (ErrorLogMuter errors =
ErrorLogMuter.substring("/my-random-prefix/their/plugin")) {
+ expectFail(() -> getPlugin("/my-random-prefix/their/plugin").call());
+ assertEquals(2, errors.getCount());
+ }
Review comment:
Two calls before (not sure why) but only one call afterwards, could we
have a comment re: why two errors for the one call?
##########
File path:
solr/core/src/test/org/apache/solr/filestore/TestDistribPackageStore.java
##########
@@ -225,6 +229,11 @@ public String toString() {
}
}
+ public static <T extends NavigableObject> T assertResponseValues(
+ Callable<T> callable, Map<String, Object> vals) throws Exception {
+ return assertResponseValues(1, callable, vals);
Review comment:
Noting that it seems that (at least some) of the callers used `10`
repeats before but now the default is `1` here.
##########
File path: solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java
##########
@@ -548,16 +473,28 @@ public void m2(SolrQueryRequest req, SolrQueryResponse
rsp) {
}
}
+ private Callable<V2Response> getPlugin(String path) {
Review comment:
Is there a `.forceV2(true)` needed here, sometimes?
`V2Request.Builder.forceV2` javadocs say "Only for testing. It's always true
otherwise" but that doesn't seem to be true actually?
```suggestion
private Callable<V2Response> getPlugin(String path, bool forceV2) {
```
##########
File path: solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java
##########
@@ -293,129 +246,101 @@ private void expectFail(ThrowingRunnable runnable)
throws Exception {
@Test
public void testApiFromPackage() throws Exception {
- MiniSolrCloudCluster cluster =
- configureCluster(4).withJettyConfig(jetty ->
jetty.enableV2(true)).configure();
String FILE1 = "/myplugin/v1.jar";
String FILE2 = "/myplugin/v2.jar";
- ContainerPluginsRegistry pluginsRegistry =
-
cluster.getOpenOverseer().getCoreContainer().getContainerPluginsRegistry();
- pluginsRegistry.setPhaser(phaser);
int version = phaser.getPhase();
String errPath = "/error/details[0]/errorMessages[0]";
Review comment:
```suggestion
```
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
