dsmiley commented on a change in pull request #2230:
URL: https://github.com/apache/lucene-solr/pull/2230#discussion_r564755969
##########
File path:
solr/core/src/test/org/apache/solr/handler/admin/AdminHandlersProxyTest.java
##########
@@ -41,7 +46,9 @@
public class AdminHandlersProxyTest extends SolrCloudTestCase {
private CloseableHttpClient httpClient;
private CloudSolrClient solrClient;
-
+ private GenericSolrRequest req;
Review comment:
Why define these here? Always define variables to the most limited
scope possible. SolrClient makes sense here because it has the lifecycle of
the test case (the class).
##########
File path:
solr/core/src/test/org/apache/solr/handler/admin/AdminHandlersProxyTest.java
##########
@@ -91,6 +98,32 @@ public void proxyMetricsHandlerAllNodes() throws
IOException, SolrServerExceptio
assertNotNull(((NamedList)nl.get(nl.getName(1))).get("metrics"));
}
+ @Test
+ public void proxyLoggingHandlerAllNodes() throws IOException,
SolrServerException {
+ CollectionAdminRequest.createCollection("collection", "conf", 2,
2).process(solrClient);
+
+ mparams = new ModifiableSolrParams();
+ mparams.set(CommonParams.QT, "/admin/logging");
+ mparams.set("nodes", "all");
+ mparams.set("set", "com.codahale.metrics.jmx.JmxReporter:WARN");
+ req = new GenericSolrRequest(SolrRequest.METHOD.GET, "/admin/logging",
mparams);
+ req.process(solrClient, "collection");
+
+ Set<String> nodes = solrClient.getClusterStateProvider().getLiveNodes();
+ nodes.forEach(node -> {
+ mparams.clear();
+ mparams.set("nodes", node);
+ req = new GenericSolrRequest(SolrRequest.METHOD.GET, "/admin/logging",
mparams);
+ try {
+ rsp = req.process(solrClient, "collection");
+ } catch (Exception e) {
+ fail("Exception while proxying request to node " + node);
Review comment:
If above you used a standard for loop, you wouldn't be forced to catch
this exception, and it'd be more succinct. Personally, I only use
Collection.forEach lambda for a one-liner or something short and no exception
propagation concern.
##########
File path:
solr/core/src/test/org/apache/solr/handler/admin/AdminHandlersProxyTest.java
##########
@@ -91,6 +98,32 @@ public void proxyMetricsHandlerAllNodes() throws
IOException, SolrServerExceptio
assertNotNull(((NamedList)nl.get(nl.getName(1))).get("metrics"));
}
+ @Test
+ public void proxyLoggingHandlerAllNodes() throws IOException,
SolrServerException {
+ CollectionAdminRequest.createCollection("collection", "conf", 2,
2).process(solrClient);
+
+ mparams = new ModifiableSolrParams();
Review comment:
BTW, Solr test infrastructure (SolrTestCaseJ4) defines a params(...)
method that is more succinct. This is fine though!
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/LoggingHandler.java
##########
@@ -49,25 +49,22 @@
@SuppressWarnings({"rawtypes"})
private LogWatcher watcher;
- private CoreContainer cc;
+ private final CoreContainer cc;
public LoggingHandler(CoreContainer cc) {
this.cc = cc;
this.watcher = cc.getLogging();
}
public LoggingHandler() {
Review comment:
Why would this constructor be used? If it's just a test, then couldn't
it use the one that takes a cc but pass null? I just did a search and see that
this constructor would be used when the handler is loaded on a core --
ImplicitPlugins.json. Ok... I suppose I should file an issue to remove these
handlers from ImplicitPlugins where they don't make sense.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
