stefan-egli commented on code in PR #1850:
URL: https://github.com/apache/jackrabbit-oak/pull/1850#discussion_r1836867533
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java:
##########
@@ -152,7 +152,7 @@ public final class DocumentNodeStore
private static final Logger LOG =
LoggerFactory.getLogger(DocumentNodeStore.class);
- private static final PerfLogger PERFLOG = new PerfLogger(
+ static final PerfLogger PERFLOG = new PerfLogger(
Review Comment:
Quite liked the approach of the the other two cases where a new static
`configurePerfLogger` is introduced. Avoids requiring this static to be made
open.
##########
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreServiceConfigurationTest.java:
##########
@@ -239,6 +239,17 @@ public void overrideFullGCExcludePaths() throws Exception {
assertArrayEquals(new String[]{"/foo", "/bar"},
config.fullGCExcludePaths());
}
+ @Test
+ public void perfLoggerInfoMillisTest() throws Exception {
+ addConfigurationEntry(preset, "perfLoggerInfoMillis", 0L);
Review Comment:
I would suggest to add a test for the default (which as mentioned above
should not be zero in my view)
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/Configuration.java:
##########
@@ -320,6 +321,13 @@
" (milliseconds).")
long recoveryDelayMillis() default DEFAULT_RECOVERY_DELAY_MILLIS;
+ @AttributeDefinition(
+ name = "Perflogger Info Millis",
+ description = "Minimum duration (in milliseconds) for an operation
that perflogger info will log. " +
Review Comment:
```suggestion
description = "Minimum duration (in milliseconds) for certain
operations that perflogger info will log. " +
```
perhaps we can emphasize that it is only "certain" operations (while not
making a definition of what certain exactly means, that can also change over
time) are perf-logged. otherwise it might imply that perflogger in general
would use this (which might or might not be the case)
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreService.java:
##########
@@ -343,6 +344,7 @@ private void registerNodeStore() throws IOException {
builder.setLeaseSocketTimeout(config.mongoLeaseSocketTimeout());
builder.setMongoDB(uri, db, config.blobCacheSize());
builder.setCollectionCompressionType(config.collectionCompressionType());
+ builder.setPerfloggerInfoMillis(config.perfLoggerInfoMillis());
Review Comment:
either this one or the one in `configureBuilder` is a duplicate. (perhaps
this one is the duplicate as the other one is also used in a few similar static
cases already?)
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/ClusterNodeInfo.java:
##########
@@ -240,6 +240,9 @@ static RecoverLockState fromString(String state) {
/** OAK-10281 : default millis to delay a recovery after a lease timeout */
static final long DEFAULT_RECOVERY_DELAY_MILLIS = 0;
+ /** OAK-11246 : default millis for perflogger info */
+ static final long DEFAULT_PERFLOGGER_INFO_MILLIS = Long.MAX_VALUE;
+
Review Comment:
don't think this belongs here, as it has nothing to do with ClusterNodeInfo.
I would suggest to move it to a more generic place, eg to
DocumentNodeStoreService
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreBranch.java:
##########
@@ -860,4 +860,8 @@ NodeState merge(@NotNull CommitHook hook,
throw ex;
}
}
+
+ static void configurePerfLogger(long infoLogMillis) {
Review Comment:
(same as above: would suggest a small javadoc here)
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/AbstractDocumentNodeState.java:
##########
@@ -162,4 +162,8 @@ private boolean revisionEquals(AbstractDocumentNodeState
other) {
return this.getLastRevision() != null
&& this.getLastRevision().equals(other.getLastRevision());
}
+
+ static void configurePerfLogger(long infoLogMillis) {
Review Comment:
I would prefer a short javadoc to explain what this does
##########
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/PerfLogger.java:
##########
@@ -50,6 +50,8 @@ public final class PerfLogger {
/** The logger to which the log statements are emitted **/
private final Logger delegate;
+ private long infoLogMillis;
Review Comment:
this should not be zero by default. with a zero as default (and without
ensuring that all PerfLoggers used throughout the code base call
`setInfoLogMillis` with their desired value) it would log everything - which
would cause a huge amount of logs. I would suggest the same value as was
already used prior to this PR, namely Long.MAX_VALUE
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreService.java:
##########
@@ -377,6 +379,10 @@ private void registerNodeStore() throws IOException {
newArrayList(gcMonitor, loggingGCMonitor)));
mkBuilder.setRevisionGCMaxAge(TimeUnit.SECONDS.toMillis(config.versionGcMaxAgeInSecs()));
+ PERFLOG.setInfoLogMillis(config.perfLoggerInfoMillis());
Review Comment:
(see above: consider also using a `configurePerfLogger` here)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
