stefan-egli commented on code in PR #1551:
URL: https://github.com/apache/jackrabbit-oak/pull/1551#discussion_r1650776449
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/Configuration.java:
##########
@@ -260,6 +260,27 @@
"are separated with '::'. Example:
-Doak.documentstore.persistentCacheIncludes=/content::/var")
String[] persistentCacheIncludes() default {"/"};
+ @AttributeDefinition(
+ name = "Full GC Include Paths",
+ description = "Paths which should be included in full garbage
collection." +
+ "Empty value means all paths are included. " +
Review Comment:
I find this potentially confusing.
Currently when no include path is specified, it results in "include
everything" - so specifying an "include everything" explicitly wouldn't be
necessary. But if we wanted to support this, then I think it shouldn't be an
empty string, but rather eg `"/"` as that is more explicit (so if someone would
specify an empty string, we could rather log a warn and skip that entry).
wdyt?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/Configuration.java:
##########
@@ -260,6 +260,27 @@
"are separated with '::'. Example:
-Doak.documentstore.persistentCacheIncludes=/content::/var")
String[] persistentCacheIncludes() default {"/"};
+ @AttributeDefinition(
+ name = "Full GC Include Paths",
+ description = "Paths which should be included in full garbage
collection." +
Review Comment:
formatting is broken as some spaces are missing after dot
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/Configuration.java:
##########
@@ -260,6 +260,27 @@
"are separated with '::'. Example:
-Doak.documentstore.persistentCacheIncludes=/content::/var")
String[] persistentCacheIncludes() default {"/"};
+ @AttributeDefinition(
+ name = "Full GC Include Paths",
+ description = "Paths which should be included in full garbage
collection." +
+ "Empty value means all paths are included. " +
+ "Any path which is added to both include & exclude paths,
" +
+ "would be removed from included paths." +
+ "Note that this value can be overridden with a system
property " +
+ "'oak.documentstore.fullGCIncludes' where paths " +
+ "are separated with '::'. Example:
-Doak.documentstore.fullGCIncludes=/content::/var")
Review Comment:
I see we sometimes state it can be overwritten via `-Doak.mongo` and
sometimes `-Doak.documentstore` ... does it have this magic indeed or is one or
the other wrong?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/Configuration.java:
##########
@@ -260,6 +260,27 @@
"are separated with '::'. Example:
-Doak.documentstore.persistentCacheIncludes=/content::/var")
String[] persistentCacheIncludes() default {"/"};
+ @AttributeDefinition(
+ name = "Full GC Include Paths",
+ description = "Paths which should be included in full garbage
collection." +
+ "Empty value means all paths are included. " +
+ "Any path which is added to both include & exclude paths,
" +
+ "would be removed from included paths." +
Review Comment:
Not sure we need to point out someone configuring duplicate paths
specifically.
What about making this comment more generic into something like "Include and
exclude paths can overlap. Exclude paths will take precedence" ?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreService.java:
##########
@@ -499,6 +499,8 @@ private void configureBuilder(DocumentNodeStoreBuilder<?>
builder) {
setDocStoreEmbeddedVerificationFeature(docStoreEmbeddedVerificationFeature).
setThrottlingEnabled(config.throttlingEnabled()).
setFullGCEnabled(config.fullGCEnabled()).
+ setFullGCIncludePaths(config.fullGCIncludes()).
Review Comment:
Configuring the identical path in both include and exclude seems weird - but
the code does behaves correctly, namely exclude has precedence, so it's "as if
that path wasn't there". But I don't think we need to add an extra warn for
this ..
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreBuilder.java:
##########
@@ -170,6 +174,8 @@ public class DocumentNodeStoreBuilder<T extends
DocumentNodeStoreBuilder<T>> {
private boolean clusterInvisible;
private boolean throttlingEnabled;
private boolean fullGCEnabled;
+ private Set<String> fullGCIncludePaths = of();
+ private Set<String> fullGCExcludePaths = of();
Review Comment:
btw, just a side-comment, I find this less readable than say `Set.of()` (but
I think this might be controversial as IDEs visualize it magically - but if you
read the raw file it is less readable for me)
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/Configuration.java:
##########
@@ -260,6 +260,27 @@
"are separated with '::'. Example:
-Doak.documentstore.persistentCacheIncludes=/content::/var")
String[] persistentCacheIncludes() default {"/"};
+ @AttributeDefinition(
+ name = "Full GC Include Paths",
+ description = "Paths which should be included in full garbage
collection." +
+ "Empty value means all paths are included. " +
+ "Any path which is added to both include & exclude paths,
" +
+ "would be removed from included paths." +
+ "Note that this value can be overridden with a system
property " +
+ "'oak.documentstore.fullGCIncludes' where paths " +
+ "are separated with '::'. Example:
-Doak.documentstore.fullGCIncludes=/content::/var")
+ String[] fullGCIncludes() default {};
+
+ @AttributeDefinition(
+ name = "Full GC Exclude Paths",
+ description = "Paths which should be excluded from full Garbage
collection." +
+ "Empty value means no paths are excluded." +
+ "Any path added to excluded list would be removed from
include paths (if present)." +
+ "Note that this value can be overridden with a system
property " +
+ "'oak.documentstore.fullGCExcludes' where paths " +
+ "are separated with '::'. Example:
-Doak.documentstore.fullGCExcludes=/content::/var")
+ String[] fullGCExcludes() default {};
Review Comment:
was wondering if the config should be called `fullGCExcludePaths` - a bit
more explicit, wdyt?
--
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: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
