Conversation
|
Two bits it would be sweet to get in
This could copy the form of the enable stats button on the Index Sets page; 
Could read Enable MongoDB Slow Logging. Note that Slow Logging has modest ongoing performance cost whilst enabled. |
moesterheld
left a comment
moesterheld
left a comment
There was a problem hiding this comment.
Thank you. I really like the InMemorySearchEngine.
I have some comments though and I think we should implement some kind of caching.
graylog2-server/src/main/java/org/graylog2/rest/resources/mongodb/MongodbClusterResource.java
Show resolved
Hide resolved
graylog2-server/src/main/java/org/graylog2/cluster/nodes/mongodb/ReplicaSetMongodbNodes.java
Outdated
Show resolved
Hide resolved
| Document query = new Document("ts", new Document("$gte", cutoffTime)) | ||
| .append("millis", new Document("$gte", 100)); // Queries taking more than 100ms | ||
|
|
||
| return mongoConnection.getDatabase("admin") |
There was a problem hiding this comment.
shouldn't this use graylog's database instead of admin, the doc says this is database specific?
graylog2-server/src/main/java/org/graylog2/cluster/nodes/mongodb/ReplicaSetMongodbNodes.java
Outdated
Show resolved
Hide resolved
graylog2-server/src/main/java/org/graylog2/cluster/nodes/mongodb/MongodbNodesProvider.java
Outdated
Show resolved
Hide resolved
graylog2-server/src/main/java/org/graylog2/utilities/lucene/LuceneInMemorySearchEngine.java
Show resolved
Hide resolved
graylog2-server/src/main/java/org/graylog2/utilities/lucene/LuceneInMemorySearchEngine.java
Outdated
Show resolved
Hide resolved
graylog2-server/src/main/java/org/graylog2/cluster/nodes/mongodb/ReplicaSetMongodbNodes.java
Outdated
Show resolved
Hide resolved
moesterheld
left a comment
moesterheld
left a comment
There was a problem hiding this comment.
looking good, but I have some comments
| public class DefaultMongodbConnectionResolver implements MongodbConnectionResolver { | ||
| @Override | ||
| public MongoClient resolve(String nodeName) { | ||
| return new MongoClient("mongodb://" + nodeName + "/?directConnection=true"); |
There was a problem hiding this comment.
do we need to consider authentication here? would this break if we need credentials for the connection?
| .collect(Collectors.toMap(host -> host, host -> runCommand(host, call)))) | ||
| .orElseGet(() -> { | ||
| final String host = connection.getClusterDescription().getServerDescriptions().getFirst().getAddress().toString(); | ||
| return Collections.singletonMap(host, runCommand(host, (h, c) -> call.apply(h, connection))); |
There was a problem hiding this comment.
I think this should be runCommand(host, call) otherwise the call.apply could create a resource leak on error
| final Sort sort = createSort(sortField, order); | ||
|
|
||
| final int start = page * perPage; | ||
| final int end = start + perPage; |
There was a problem hiding this comment.
shouldn't this be offset+page? If page is 1 and perPage is 10, this would retrieve 20 results and then limit that again to perPage using offset?
| } else if (operator == SearchQueryOperators.EQUALS) { | ||
| return new TermQuery(new Term(fieldName, stringValue)); | ||
| } else if (operator == SearchQueryOperators.GREATER) { | ||
| return TermRangeQuery.newStringRange(fieldName, stringValue, null, false, false); |
There was a problem hiding this comment.
TermRangeQuery won't produce correct results for >(=) and <(=) because it does string comparison. should we check if the value is a number first and then do a numeric comparison?
| } | ||
|
|
||
| public LuceneDocBuilder boolVal(String key, Boolean value) { | ||
| return intVal(key, value ? 1 : 0); |
There was a problem hiding this comment.
this is the only method without a null check
| return pointsConfig; | ||
| } | ||
|
|
||
| private static String wrapEmptyQuery(String queryString) { |
| } | ||
|
|
||
| @Nonnull | ||
| private StandardQueryParser createQueryParser(Analyzer analyzer) { |
| } | ||
|
|
||
| @Nonnull | ||
| private Map<String, PointsConfig> getStringPointsConfigMap() { |
There was a problem hiding this comment.
only used in createQueryParser, can be removed
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import java.util.Collections; |
| return 100.0d * fsUsedSize / fsTotalSize; | ||
| } | ||
| } catch (Exception e) { | ||
| // Stats may not be available or accessible |
4 participants
/nocl yet
fixes: https://github.com/Graylog2/graylog-plugin-enterprise/issues/13215
Description
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: