Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
rishabhdaim merged PR #1551: URL: https://github.com/apache/jackrabbit-oak/pull/1551 -- 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
Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
rishabhdaim commented on code in PR #1551:
URL: https://github.com/apache/jackrabbit-oak/pull/1551#discussion_r1650892053
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreServiceConfigurationTest.java:
##
@@ -83,7 +84,9 @@ public void defaultValues() throws Exception {
assertEquals("MONGO", config.documentStoreType());
assertEquals(DocumentNodeStoreService.DEFAULT_BUNDLING_DISABLED,
config.bundlingDisabled());
assertEquals(DocumentMK.Builder.DEFAULT_UPDATE_LIMIT,
config.updateLimit());
-assertEquals(Arrays.asList("/"),
Arrays.asList(config.persistentCacheIncludes()));
+assertEquals(of("/"), Arrays.asList(config.persistentCacheIncludes()));
Review Comment:
Yes, it can be avoided, but IntelliJ insisted on making this change.
--
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
Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
stefan-egli commented on code in PR #1551:
URL: https://github.com/apache/jackrabbit-oak/pull/1551#discussion_r1650864494
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreServiceConfigurationTest.java:
##
@@ -83,7 +84,9 @@ public void defaultValues() throws Exception {
assertEquals("MONGO", config.documentStoreType());
assertEquals(DocumentNodeStoreService.DEFAULT_BUNDLING_DISABLED,
config.bundlingDisabled());
assertEquals(DocumentMK.Builder.DEFAULT_UPDATE_LIMIT,
config.updateLimit());
-assertEquals(Arrays.asList("/"),
Arrays.asList(config.persistentCacheIncludes()));
+assertEquals(of("/"), Arrays.asList(config.persistentCacheIncludes()));
Review Comment:
nitpick : looks unrelated/unnecessary?
--
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
Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
rishabhdaim commented on code in PR #1551:
URL: https://github.com/apache/jackrabbit-oak/pull/1551#discussion_r1650849974
##
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:
fixed in
https://github.com/apache/jackrabbit-oak/pull/1551/commits/55f310e2d65cfe3208ff7688ce93787eccb0cfba
##
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:
fixed in
https://github.com/apache/jackrabbit-oak/pull/1551/commits/55f310e2d65cfe3208ff7688ce93787eccb0cfba
##
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:
fixed in
https://github.com/apache/jackrabbit-oak/pull/1551/commits/55f310e2d65cfe3208ff7688ce93787eccb0cfba
--
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
Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
rishabhdaim commented on code in PR #1551:
URL: https://github.com/apache/jackrabbit-oak/pull/1551#discussion_r1650849122
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreBuilder.java:
##
@@ -170,6 +174,8 @@ public class DocumentNodeStoreBuilder> {
private boolean clusterInvisible;
private boolean throttlingEnabled;
private boolean fullGCEnabled;
+private Set fullGCIncludePaths = of();
+private Set fullGCExcludePaths = of();
Review Comment:
fixed in
https://github.com/apache/jackrabbit-oak/pull/1551/commits/55f310e2d65cfe3208ff7688ce93787eccb0cfba
##
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:
fixed in
https://github.com/apache/jackrabbit-oak/pull/1551/commits/55f310e2d65cfe3208ff7688ce93787eccb0cfba
##
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:
fixed in
https://github.com/apache/jackrabbit-oak/pull/1551/commits/55f310e2d65cfe3208ff7688ce93787eccb0cfba
--
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
Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
rishabhdaim commented on code in PR #1551:
URL: https://github.com/apache/jackrabbit-oak/pull/1551#discussion_r1650825750
##
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:
This magic happens here:
https://github.com/apache/jackrabbit-oak/blob/trunk/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreServiceConfiguration.java#L123
--
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
Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
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> {
private boolean clusterInvisible;
private boolean throttlingEnabled;
private boolean fullGCEnabled;
+private Set fullGCIncludePaths = of();
+private Set fullGCExcludePaths = of();
Review Comment:
btw, just a side-comment, I
Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
rishabhdaim commented on code in PR #1551:
URL: https://github.com/apache/jackrabbit-oak/pull/1551#discussion_r1650721462
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentNodeStoreBuilderTest.java:
##
@@ -67,6 +68,20 @@ public void fullGCDisabled() {
assertFalse(builder.isFullGCEnabled());
}
+@Test
Review Comment:
added in
https://github.com/apache/jackrabbit-oak/pull/1551/commits/167b46c5eaa518d39bad952853ac55bed2e0b7a5
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentNodeStoreBuilderTest.java:
##
@@ -60,6 +62,22 @@ public void fullGCFeatureToggleDisabled() {
assertNull(builder.getDocStoreFullGCFeature());
}
+@Test
Review Comment:
added in
https://github.com/apache/jackrabbit-oak/pull/1551/commits/167b46c5eaa518d39bad952853ac55bed2e0b7a5
--
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
Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
rishabhdaim commented on code in PR #1551:
URL: https://github.com/apache/jackrabbit-oak/pull/1551#discussion_r1650713278
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreServiceConfigurationTest.java:
##
@@ -115,6 +118,22 @@ public void fullGCEnabled() throws Exception {
assertEquals(fullGCDocStore, config.fullGCEnabled());
}
+@Test
Review Comment:
Check for invalid paths is added here:
https://github.com/apache/jackrabbit-oak/pull/1551/commits/772c8559d4fdfa7e9fd8d251272f18606325ca04
--
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
Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
rishabhdaim commented on code in PR #1551:
URL: https://github.com/apache/jackrabbit-oak/pull/1551#discussion_r1650697389
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreServiceConfigurationTest.java:
##
@@ -115,6 +118,22 @@ public void fullGCEnabled() throws Exception {
assertEquals(fullGCDocStore, config.fullGCEnabled());
}
+@Test
Review Comment:
> I believe both lists should be different, with even one common path,
nothing gets fetched from DB. cc @stefan-egli
my above is wrong, only common path is excluded not all.
--
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
Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
rishabhdaim commented on code in PR #1551:
URL: https://github.com/apache/jackrabbit-oak/pull/1551#discussion_r1650695069
##
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:
I have tested on local and in case a path is present in both `include` &
`exclude`, it won't be selected.
--
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
Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
rishabhdaim commented on code in PR #1551:
URL: https://github.com/apache/jackrabbit-oak/pull/1551#discussion_r1650412061
##
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:
That's a good catch. I have tested with a path in both the `include` &
`exclude` list and it turns out that in that case, we won't be fetching any
document from db.
cc @stefan-egli
--
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
Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
rishabhdaim commented on code in PR #1551:
URL: https://github.com/apache/jackrabbit-oak/pull/1551#discussion_r1650412889
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreServiceConfigurationTest.java:
##
@@ -115,6 +118,22 @@ public void fullGCEnabled() throws Exception {
assertEquals(fullGCDocStore, config.fullGCEnabled());
}
+@Test
Review Comment:
I believe both lists should be different, with even one common path, nothing
gets fetched from DB.
cc @stefan-egli
--
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
Re: [PR] OAK-10912 : make fullGc include/exclude paths configurable via OSGI config [jackrabbit-oak]
rishabhdaim commented on code in PR #1551:
URL: https://github.com/apache/jackrabbit-oak/pull/1551#discussion_r1650412061
##
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:
That's a good catch. I have tested with a path in both the `include` &
`exclude` list and it turns out that in that case, we won't be fetching any
document from db.
cc @stefan-egli
--
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
