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.

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

Jump to bottom

SOLR-15011: /admin/logging handler is configured logs to all nodes #2230

Merged
merged 8 commits into from Feb 3, 2021
Merged

SOLR-15011: /admin/logging handler is configured logs to all nodes #2230

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e1ef421
SOLR-15011: /admin/logging handler is configured logs to all nodes
Jan 21, 2021
cf46271
SOLR-15011: Add a test
Jan 26, 2021
3021391
SOLR-15011: Update the test
Jan 27, 2021
1c0f5a1
Update changes.txt and configuring-logging.adoc
Jan 27, 2021
e83fe1a
Add nodes=all to setLevel only
Feb 1, 2021
936d090
Change the log level to debug
Feb 1, 2021
b9aec5b
Use params for nodes=all
Feb 1, 2021
94b69a4
Update solr/CHANGES.txt
dsmiley Feb 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion solr/core/src/java/org/apache/solr/handler/admin/LoggingHandler.java
View file
Open in desktop
Expand Up @@ -49,13 +49,15 @@ public class LoggingHandler extends RequestHandlerBase implements SolrCoreAware

@SuppressWarnings({"rawtypes"})
private LogWatcher watcher;
private final CoreContainer cc;

public LoggingHandler(CoreContainer cc) {
this.cc = cc;
this.watcher = cc.getLogging();
}

public LoggingHandler() {
dsmiley marked this conversation as resolved.
Show resolved Hide resolved

this.cc = null;
}

@Override
Expand Down Expand Up @@ -151,6 +153,9 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
rsp.add("loggers", info);
}
rsp.setHttpCaching(false);
if (cc != null && AdminHandlersProxy.maybeProxyToNodes(req, rsp, cc)) {
dsmiley marked this conversation as resolved.
Show resolved Hide resolved
return; // Request was proxied to other node
}
}

// ////////////////////// SolrInfoMBeans methods //////////////////////
Expand Down
35 changes: 34 additions & 1 deletion solr/core/src/test/org/apache/solr/handler/admin/AdminHandlersProxyTest.java
View file
Open in desktop
Expand Up @@ -18,6 +18,7 @@
package org.apache.solr.handler.admin;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Set;
import java.util.concurrent.TimeUnit;
Expand All @@ -27,12 +28,16 @@
import org.apache.solr.client.solrj.SolrRequest;
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.impl.CloudSolrClient;
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
import org.apache.solr.client.solrj.request.GenericSolrRequest;
import org.apache.solr.client.solrj.response.SimpleSolrResponse;
import org.apache.solr.cloud.SolrCloudTestCase;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.params.CommonParams;
import org.apache.solr.common.params.MapSolrParams;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.common.util.SimpleOrderedMap;
import org.junit.After;
import org.junit.Before;
import org.junit.BeforeClass;
Expand All @@ -41,7 +46,9 @@
public class AdminHandlersProxyTest extends SolrCloudTestCase {
private CloseableHttpClient httpClient;
private CloudSolrClient solrClient;

private GenericSolrRequest req;
dsmiley marked this conversation as resolved.
Show resolved Hide resolved
private SimpleSolrResponse rsp;
private ModifiableSolrParams mparams;
@BeforeClass
public static void setupCluster() throws Exception {
System.setProperty("metricsEnabled", "true");
Expand Down Expand Up @@ -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();
dsmiley marked this conversation as resolved.
Show resolved Hide resolved
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);
dsmiley marked this conversation as resolved.
Show resolved Hide resolved
}
NamedList<Object> nl = rsp.getResponse();
assertEquals("WARN", ((SimpleOrderedMap) ((ArrayList)nl.get("loggers")).get(5)).get("level"));
});
}

@Test(expected = SolrException.class)
public void proxySystemInfoHandlerNonExistingNode() throws IOException, SolrServerException {
MapSolrParams params = new MapSolrParams(Collections.singletonMap("nodes", "example.com:1234_solr"));
Expand Down
2 changes: 1 addition & 1 deletion solr/webapp/web/js/angular/services.js
View file
Open in desktop
Expand Up @@ -58,7 +58,7 @@ solrAdminServices.factory('System',
}])
.factory('Logging',
['$resource', function($resource) {
return $resource('admin/info/logging', {'wt':'json', '_':Date.now()}, {
return $resource('admin/info/logging', {'wt':'json', 'nodes': 'all', '_':Date.now()}, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very unfamiliar with the admin UI code structure. Can you explain why nodes=all needed to be set in two places?

Copy link
Contributor

Choose a reason for hiding this comment

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

@NazerkeBS ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsmiley, I set nodes=all to only one place which is in services.js file

Copy link
Contributor

@dsmiley dsmiley Jan 27, 2021 •
edited

Choose a reason for hiding this comment

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

Okay; I see you improved that from an earlier commit.

@janhoy maybe you could take a look at this please. nodes=all is intended only for when setLevel is submitted, not for all logging handler interactions. Do you think this is done correctly here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think nodes=all should be added only to the setLevel call. Should be easy to check in browser debugger that it sends correct request.

Copy link
Contributor

Choose a reason for hiding this comment

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

I verified this is a problem now! Thanks for the tip Jan; I forgot how easy it is to verify :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank Jan and David! Just updated the PR.

"events": {params: {since:'0'}},
"levels": {},
"setLevel": {}
Expand Down