Re: [PR] OAK-10896: - Add OSGI configuration variable for removal of deleted properties and orphaned nodes [jackrabbit-oak]
shodaaan commented on code in PR #1539:
URL: https://github.com/apache/jackrabbit-oak/pull/1539#discussion_r1651122982
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/util/UtilsTest.java:
##
@@ -236,6 +237,30 @@ public void fullGCDisabledForRDB() {
assertFalse("Full GC is disabled for RDB Document Store",
fullGCEnabled);
}
+@Test
+public void fullGCModeDefaultValue() {
+int fullGCModeDefaultValue =
getFullGCMode(newDocumentNodeStoreBuilder());
+final int FULL_GC_MODE_NONE = 0;
Review Comment:
Done.
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/util/UtilsTest.java:
##
@@ -236,6 +237,30 @@ public void fullGCDisabledForRDB() {
assertFalse("Full GC is disabled for RDB Document Store",
fullGCEnabled);
}
+@Test
+public void fullGCModeDefaultValue() {
+int fullGCModeDefaultValue =
getFullGCMode(newDocumentNodeStoreBuilder());
+final int FULL_GC_MODE_NONE = 0;
+assertEquals("Full GC mode has NONE value by default",
fullGCModeDefaultValue, FULL_GC_MODE_NONE);
+}
+
+@Test
+public void fullGCModeSetViaConfiguration() {
+DocumentNodeStoreBuilder builder = newDocumentNodeStoreBuilder();
+final int FULL_GC_MODE_GAP_ORPHANS = 2;
Review Comment:
Done.
--
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-10896: - Add OSGI configuration variable for removal of deleted properties and orphaned nodes [jackrabbit-oak]
shodaaan commented on code in PR #1539:
URL: https://github.com/apache/jackrabbit-oak/pull/1539#discussion_r1651122588
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java:
##
@@ -1009,6 +1009,16 @@ public static boolean
isEmbeddedVerificationEnabled(final DocumentNodeStoreBuild
return builder.isEmbeddedVerificationEnabled() ||
(docStoreEmbeddedVerificationFeature != null &&
docStoreEmbeddedVerificationFeature.isEnabled());
}
+/**
+ * Returns the full GC mode value for the DocumentNodeStore.
+ *
+ * @param builder instance for DocumentNodeStoreBuilder
+ * @return true if full GC is enabled else false
+ */
+public static int getFullGCMode(final DocumentNodeStoreBuilder builder)
{
Review Comment:
Fixed.
--
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-10896: - Add OSGI configuration variable for removal of deleted properties and orphaned nodes [jackrabbit-oak]
shodaaan commented on code in PR #1539: URL: https://github.com/apache/jackrabbit-oak/pull/1539#discussion_r165738 ## oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java: ## @@ -47,13 +47,13 @@ import org.apache.jackrabbit.oak.plugins.document.NodeDocument; import org.apache.jackrabbit.oak.plugins.document.RevisionContextWrapper; import org.apache.jackrabbit.oak.plugins.document.SweepHelper; +import org.apache.jackrabbit.oak.plugins.document.VersionGCOptions; import org.apache.jackrabbit.oak.plugins.document.VersionGCSupport; +import org.apache.jackrabbit.oak.plugins.document.VersionGarbageCollector; import org.apache.jackrabbit.oak.plugins.document.VersionGarbageCollector.VersionGCInfo; import org.apache.jackrabbit.oak.plugins.document.VersionGarbageCollector.VersionGCStats; import org.apache.jackrabbit.oak.plugins.document.util.MongoConnection; import org.apache.jackrabbit.oak.run.commons.Command; -import org.apache.jackrabbit.oak.plugins.document.VersionGCOptions; -import org.apache.jackrabbit.oak.plugins.document.VersionGarbageCollector; Review Comment: done -- 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
[PR] OAK-10917 - Make persistent cache related arguments optional for Azur… [jackrabbit-oak]
dulceanu opened a new pull request, #1555: URL: https://github.com/apache/jackrabbit-oak/pull/1555 …e compaction -- 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-10896: - Add OSGI configuration variable for removal of deleted properties and orphaned nodes [jackrabbit-oak]
shodaaan commented on code in PR #1539:
URL: https://github.com/apache/jackrabbit-oak/pull/1539#discussion_r1651098372
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java:
##
@@ -2603,7 +2605,7 @@ private void cleanOrphanedBranches() {
}
private void cleanRootCollisions() {
-String id = Utils.getIdFromPath(ROOT);
+String id = getIdFromPath(ROOT);
Review Comment:
Fixed.
--
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-10896: - Add OSGI configuration variable for removal of deleted properties and orphaned nodes [jackrabbit-oak]
shodaaan commented on code in PR #1539:
URL: https://github.com/apache/jackrabbit-oak/pull/1539#discussion_r1651096514
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java:
##
@@ -692,7 +694,7 @@ public int getMemory() {
diffCache = builder.getDiffCache(this.clusterId);
// check if root node exists
-NodeDocument rootDoc = store.find(NODES, Utils.getIdFromPath(ROOT));
+NodeDocument rootDoc = store.find(NODES, getIdFromPath(ROOT));
Review Comment:
Fixed.
--
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-10896: - Add OSGI configuration variable for removal of deleted properties and orphaned nodes [jackrabbit-oak]
rishabhdaim commented on code in PR #1539:
URL: https://github.com/apache/jackrabbit-oak/pull/1539#discussion_r1651080682
##
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##
@@ -69,10 +69,11 @@
import static org.apache.jackrabbit.oak.plugins.document.Collection.NODES;
import static
org.apache.jackrabbit.oak.plugins.document.DocumentNodeStoreHelper.createVersionGC;
import static
org.apache.jackrabbit.oak.plugins.document.FormatVersion.versionOf;
+import static
org.apache.jackrabbit.oak.plugins.document.util.Utils.getFullGCMode;
import static
org.apache.jackrabbit.oak.plugins.document.util.Utils.getIdFromPath;
import static
org.apache.jackrabbit.oak.plugins.document.util.Utils.getRootDocument;
-import static
org.apache.jackrabbit.oak.plugins.document.util.Utils.isFullGCEnabled;
Review Comment:
same as above.
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##
@@ -211,6 +211,29 @@ static FullGCMode getFullGcMode() {
return fullGcMode;
}
+/**
+ * Set the full GC mode to be used according to the provided configuration
value.
+ * The configuration value will be ignored and fullGCMode will be reset to
NONE
+ * if it is set to any other values than the supported ones.
+ * @param fullGcModeConfig
+ */
+static void setFullGcMode(int fullGcModeConfig) {
Review Comment:
I would suggest renaming variable name to `fullGcMode`
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/util/UtilsTest.java:
##
@@ -236,6 +237,30 @@ public void fullGCDisabledForRDB() {
assertFalse("Full GC is disabled for RDB Document Store",
fullGCEnabled);
}
+@Test
+public void fullGCModeDefaultValue() {
+int fullGCModeDefaultValue =
getFullGCMode(newDocumentNodeStoreBuilder());
+final int FULL_GC_MODE_NONE = 0;
Review Comment:
please use camelCase here.
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java:
##
@@ -1009,6 +1009,16 @@ public static boolean
isEmbeddedVerificationEnabled(final DocumentNodeStoreBuild
return builder.isEmbeddedVerificationEnabled() ||
(docStoreEmbeddedVerificationFeature != null &&
docStoreEmbeddedVerificationFeature.isEnabled());
}
+/**
+ * Returns the full GC mode value for the DocumentNodeStore.
+ *
+ * @param builder instance for DocumentNodeStoreBuilder
+ * @return true if full GC is enabled else false
+ */
+public static int getFullGCMode(final DocumentNodeStoreBuilder builder)
{
Review Comment:
This utility method doesn't make sense, it only returns `fullGcMode` from
the `builder` object. I would remove this.
##
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##
@@ -47,13 +47,13 @@
import org.apache.jackrabbit.oak.plugins.document.NodeDocument;
import org.apache.jackrabbit.oak.plugins.document.RevisionContextWrapper;
import org.apache.jackrabbit.oak.plugins.document.SweepHelper;
+import org.apache.jackrabbit.oak.plugins.document.VersionGCOptions;
import org.apache.jackrabbit.oak.plugins.document.VersionGCSupport;
+import org.apache.jackrabbit.oak.plugins.document.VersionGarbageCollector;
import
org.apache.jackrabbit.oak.plugins.document.VersionGarbageCollector.VersionGCInfo;
import
org.apache.jackrabbit.oak.plugins.document.VersionGarbageCollector.VersionGCStats;
import org.apache.jackrabbit.oak.plugins.document.util.MongoConnection;
import org.apache.jackrabbit.oak.run.commons.Command;
-import org.apache.jackrabbit.oak.plugins.document.VersionGCOptions;
-import org.apache.jackrabbit.oak.plugins.document.VersionGarbageCollector;
Review Comment:
please revert these import changes.
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/util/UtilsTest.java:
##
@@ -236,6 +237,30 @@ public void fullGCDisabledForRDB() {
assertFalse("Full GC is disabled for RDB Document Store",
fullGCEnabled);
}
+@Test
+public void fullGCModeDefaultValue() {
+int fullGCModeDefaultValue =
getFullGCMode(newDocumentNodeStoreBuilder());
+final int FULL_GC_MODE_NONE = 0;
+assertEquals("Full GC mode has NONE value by default",
fullGCModeDefaultValue, FULL_GC_MODE_NONE);
+}
+
+@Test
+public void fullGCModeSetViaConfiguration() {
+DocumentNodeStoreBuilder builder = newDocumentNodeStoreBuilder();
+final int FULL_GC_MODE_GAP_ORPHANS = 2;
Review Comment:
same as above.
--
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:
[PR] OAK-10843 : exclude a flaky test combination [jackrabbit-oak]
stefan-egli opened a new pull request, #1554: URL: https://github.com/apache/jackrabbit-oak/pull/1554 (no comment) -- 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 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-10896: - Add OSGI configuration variable for removal of deleted properties and orphaned nodes [jackrabbit-oak]
stefan-egli commented on code in PR #1539:
URL: https://github.com/apache/jackrabbit-oak/pull/1539#discussion_r1650969435
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java:
##
@@ -692,7 +694,7 @@ public int getMemory() {
diffCache = builder.getDiffCache(this.clusterId);
// check if root node exists
-NodeDocument rootDoc = store.find(NODES, Utils.getIdFromPath(ROOT));
+NodeDocument rootDoc = store.find(NODES, getIdFromPath(ROOT));
Review Comment:
but it's in a random unrelated code - which is against our goal to not do
unrelated changes
--
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-10896: - Add feature toggle for removal of deleted properties and orphaned nodes [jackrabbit-oak]
shodaaan commented on code in PR #1539:
URL: https://github.com/apache/jackrabbit-oak/pull/1539#discussion_r1650889773
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/VersionGCTest.java:
##
@@ -531,6 +531,57 @@ public void testResetFullGCDryRunMode() throws Exception {
// OAK-10370 END
+// OAK-10896
+
+@Test
+public void testVersionGCLoadGCModeConfigurationNotApplicable() throws
Exception {
+int fullGcModeNotAllowedValue = 5;
+int fullGcModeGapOrphans = 2;
+
+// set fullGcMode to allowed value that is different than NONE
+VersionGarbageCollector.setFullGcMode(fullGcModeGapOrphans);
+
+// reinitialize VersionGarbageCollector with not allowed value
+VersionGarbageCollector gc = new VersionGarbageCollector(
+ns, new VersionGCSupport(store), true, false, false,
+fullGcModeNotAllowedValue);
+
+assertEquals("Starting VersionGarbageCollector with not applicable /
not allowed value" +
+"will set fullGcMode to default NONE", gc.getFullGcMode(),
VersionGarbageCollector.FullGCMode.NONE);
Review Comment:
Fixed the comment, unit tests and static change, thank you for the
suggestions
--
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-10896: - Add feature toggle for removal of deleted properties and orphaned nodes [jackrabbit-oak]
shodaaan commented on code in PR #1539:
URL: https://github.com/apache/jackrabbit-oak/pull/1539#discussion_r1650887782
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreService.java:
##
@@ -238,6 +239,8 @@ static DocumentStoreType fromString(String type) {
private Feature cancelInvalidationFeature;
private Feature docStoreFullGCFeature;
private Feature docStoreEmbeddedVerificationFeature;
+private Feature docStoreFullGCModeGapOrphansFeature;
+private Feature docStoreFullGCModeEmptyPropertiesFeature;
Review Comment:
Fixed, thank you for the comment.
--
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-10896: - Add feature toggle for removal of deleted properties and orphaned nodes [jackrabbit-oak]
shodaaan commented on code in PR #1539:
URL: https://github.com/apache/jackrabbit-oak/pull/1539#discussion_r1650887172
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java:
##
@@ -692,7 +694,7 @@ public int getMemory() {
diffCache = builder.getDiffCache(this.clusterId);
// check if root node exists
-NodeDocument rootDoc = store.find(NODES, Utils.getIdFromPath(ROOT));
+NodeDocument rootDoc = store.find(NODES, getIdFromPath(ROOT));
Review Comment:
This is consistent with how we are using static method calls. The consistent
approach would be IMHO to actually do this everywhere, but I think a
refactoring ticket is the best place for that.
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java:
##
@@ -716,7 +718,7 @@ public int getMemory() {
setRoot(head);
// make sure _lastRev is written back to store
backgroundWrite();
-rootDoc = store.find(NODES, Utils.getIdFromPath(ROOT));
+rootDoc = store.find(NODES, getIdFromPath(ROOT));
Review Comment:
This is consistent with how we are using static method calls. The consistent
approach would be IMHO to actually do this everywhere, but I think a
refactoring ticket is the best place for that.
--
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-10748: Improve statistics to collect which type of garbage is sent/deleted [jackrabbit-oak]
Joscorbe commented on PR #1543: URL: https://github.com/apache/jackrabbit-oak/pull/1543#issuecomment-2186350396 I will squash this PR into a single commit once approved. -- 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-10914 : disable mongo-server-side exclude paths until query perfo… [jackrabbit-oak]
stefan-egli merged PR #1553: URL: https://github.com/apache/jackrabbit-oak/pull/1553 -- 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-10748: Improve statistics to collect which type of garbage is sent/deleted [jackrabbit-oak]
Joscorbe commented on code in PR #1543:
URL: https://github.com/apache/jackrabbit-oak/pull/1543#discussion_r1650867475
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##
@@ -240,7 +240,21 @@ static FullGCMode getFullGcMode() {
AUDIT_LOG.info(" VersionGarbageCollector created with fullGcMode
= {}", fullGcMode);
}
-public void setStatisticsProvider(StatisticsProvider provider) {
+/**
+ * Please note that at the moment the includes do not
+ * take long paths into account. That is, if a long path was
+ * supposed to be included via an include, it is not.
+ * Reason for this is that long paths would require
+ * the mongo query to include a '_path' condition - which disallows
+ * mongo from using the '_modified_id' index. IOW long paths
+ * would result in full scans - which results in bad performance.
+ */
+void setFullGCPaths(@NotNull Set includes, @NotNull Set
excludes) {
+this.fullGCIncludePaths = requireNonNull(includes);
+this.fullGCExcludePaths = requireNonNull(excludes);
+}
+
+void setStatisticsProvider(StatisticsProvider provider) {
Review Comment:
Oh, the public was lost 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: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10914 : disable mongo-server-side exclude paths until query perfo… [jackrabbit-oak]
stefan-egli commented on PR #1553: URL: https://github.com/apache/jackrabbit-oak/pull/1553#issuecomment-2186346121 > I think we might need to update the test for this as well. Client-side incl/exclude always also applies (perhaps something to improve at some point), so if mongo doesn't do server-side exclude, the client-side still applies, so tests still run fine. -- 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-10914 : disable mongo-server-side exclude paths until query perfo… [jackrabbit-oak]
rishabhdaim commented on PR #1553: URL: https://github.com/apache/jackrabbit-oak/pull/1553#issuecomment-2186335794 I think we might need to update the test for this as well. -- 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
[PR] OAK-10914 : disable mongo-server-side exclude paths until query perfo… [jackrabbit-oak]
stefan-egli opened a new pull request, #1553: URL: https://github.com/apache/jackrabbit-oak/pull/1553 …rmance is fixed -- 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-10748: Improve statistics to collect which type of garbage is sent/deleted [jackrabbit-oak]
Joscorbe commented on PR #1543: URL: https://github.com/apache/jackrabbit-oak/pull/1543#issuecomment-2186329449 > actually, there are compilation errors: > > ``` > [INFO] - > Error: COMPILATION ERROR : > [INFO] - > Error: /home/runner/work/jackrabbit-oak/jackrabbit-oak/oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:[367,11] setStatisticsProvider(org.apache.jackrabbit.oak.stats.StatisticsProvider) is not public in org.apache.jackrabbit.oak.plugins.document.VersionGarbageCollector; cannot be accessed from outside package > Error: /home/runner/work/jackrabbit-oak/jackrabbit-oak/oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:[538,11] setStatisticsProvider(org.apache.jackrabbit.oak.stats.StatisticsProvider) is not public in org.apache.jackrabbit.oak.plugins.document.VersionGarbageCollector; cannot be accessed from outside package > [INFO] 2 errors > ``` I will double-check this, sounds weird, I built it and ran all the integration tests locally... -- 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-10748: Improve statistics to collect which type of garbage is sent/deleted [jackrabbit-oak]
Joscorbe commented on code in PR #1543:
URL: https://github.com/apache/jackrabbit-oak/pull/1543#discussion_r1650854809
##
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##
@@ -535,8 +544,8 @@ private void collectDocument(RevisionsOptions options,
Closer closer, String pat
}
gc.collectGarbageOnDocument(documentNodeStore, workingDocument,
options.isVerbose());
-//TODO: Probably we should output some details of fullGCStats. Could
be done after OAK-10378
-//gc.getFullGCStats();
+System.out.println("Full GC Stats:");
+System.out.println(gc.getFullGCStatsReport());
Review Comment:
Not on purpose, I will add them here too.
--
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-10748: Improve statistics to collect which type of garbage is sent/deleted [jackrabbit-oak]
stefan-egli commented on PR #1543: URL: https://github.com/apache/jackrabbit-oak/pull/1543#issuecomment-2186287106 actually, there are compilation errors: ``` [INFO] - Error: COMPILATION ERROR : [INFO] - Error: /home/runner/work/jackrabbit-oak/jackrabbit-oak/oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:[367,11] setStatisticsProvider(org.apache.jackrabbit.oak.stats.StatisticsProvider) is not public in org.apache.jackrabbit.oak.plugins.document.VersionGarbageCollector; cannot be accessed from outside package Error: /home/runner/work/jackrabbit-oak/jackrabbit-oak/oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:[538,11] setStatisticsProvider(org.apache.jackrabbit.oak.stats.StatisticsProvider) is not public in org.apache.jackrabbit.oak.plugins.document.VersionGarbageCollector; cannot be accessed from outside package [INFO] 2 errors ``` -- 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-10748: Improve statistics to collect which type of garbage is sent/deleted [jackrabbit-oak]
stefan-egli commented on code in PR #1543:
URL: https://github.com/apache/jackrabbit-oak/pull/1543#discussion_r1650820289
##
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##
@@ -535,8 +544,8 @@ private void collectDocument(RevisionsOptions options,
Closer closer, String pat
}
gc.collectGarbageOnDocument(documentNodeStore, workingDocument,
options.isVerbose());
-//TODO: Probably we should output some details of fullGCStats. Could
be done after OAK-10378
-//gc.getFullGCStats();
+System.out.println("Full GC Stats:");
+System.out.println(gc.getFullGCStatsReport());
Review Comment:
just curious : above the report is indented with 4 spaces but not here - is
this on purpose?
--
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-10896: - Add feature toggle for removal of deleted properties and orphaned nodes [jackrabbit-oak]
stefan-egli commented on PR #1539: URL: https://github.com/apache/jackrabbit-oak/pull/1539#issuecomment-2186194768 PS: I think we should adjust jira/pr titles to talk about osgi config instead of feature toggle (to avoid future confusion) -- 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-10896: - Add feature toggle for removal of deleted properties and orphaned nodes [jackrabbit-oak]
stefan-egli commented on code in PR #1539:
URL: https://github.com/apache/jackrabbit-oak/pull/1539#discussion_r1650729569
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java:
##
@@ -692,7 +694,7 @@ public int getMemory() {
diffCache = builder.getDiffCache(this.clusterId);
// check if root node exists
-NodeDocument rootDoc = store.find(NODES, Utils.getIdFromPath(ROOT));
+NodeDocument rootDoc = store.find(NODES, getIdFromPath(ROOT));
Review Comment:
I think we should try to avoid making unrelated changes like these, they
make the PR change surface unnecessarily larger (and in this particular case I
don't think it is an improvement - but I guess that is a controversial topic)
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java:
##
@@ -716,7 +718,7 @@ public int getMemory() {
setRoot(head);
// make sure _lastRev is written back to store
backgroundWrite();
-rootDoc = store.find(NODES, Utils.getIdFromPath(ROOT));
+rootDoc = store.find(NODES, getIdFromPath(ROOT));
Review Comment:
(same as above)
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreService.java:
##
@@ -238,6 +239,8 @@ static DocumentStoreType fromString(String type) {
private Feature cancelInvalidationFeature;
private Feature docStoreFullGCFeature;
private Feature docStoreEmbeddedVerificationFeature;
+private Feature docStoreFullGCModeGapOrphansFeature;
+private Feature docStoreFullGCModeEmptyPropertiesFeature;
Review Comment:
I think we aimed to remove the features here?
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/CommitBuilderTest.java:
##
@@ -83,8 +83,8 @@ public void buildWithNullRevision() {
CommitBuilder builder = new CommitBuilder(ns, null);
try {
builder.build(null);
-expectNPE();
-} catch (NullPointerException e) {
+expectIllegalArgumentException();
+} catch (IllegalArgumentException e) {
Review Comment:
it fails with a NPE for me though
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/VersionGCTest.java:
##
@@ -531,6 +531,57 @@ public void testResetFullGCDryRunMode() throws Exception {
// OAK-10370 END
+// OAK-10896
+
+@Test
+public void testVersionGCLoadGCModeConfigurationNotApplicable() throws
Exception {
+int fullGcModeNotAllowedValue = 5;
+int fullGcModeGapOrphans = 2;
+
+// set fullGcMode to allowed value that is different than NONE
+VersionGarbageCollector.setFullGcMode(fullGcModeGapOrphans);
+
+// reinitialize VersionGarbageCollector with not allowed value
+VersionGarbageCollector gc = new VersionGarbageCollector(
+ns, new VersionGCSupport(store), true, false, false,
+fullGcModeNotAllowedValue);
+
+assertEquals("Starting VersionGarbageCollector with not applicable /
not allowed value" +
+"will set fullGcMode to default NONE", gc.getFullGcMode(),
VersionGarbageCollector.FullGCMode.NONE);
Review Comment:
```suggestion
"will set fullGcMode to default NONE",
VersionGarbageCollector.getFullGcMode(),
VersionGarbageCollector.FullGCMode.NONE);
```
static method should be accessed in a static way
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java:
##
@@ -2603,7 +2605,7 @@ private void cleanOrphanedBranches() {
}
private void cleanRootCollisions() {
-String id = Utils.getIdFromPath(ROOT);
+String id = getIdFromPath(ROOT);
Review Comment:
(same as above throughout this class)
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##
@@ -1092,61 +1124,63 @@ public void collectGarbage(final NodeDocument doc,
final GCPhases phases) {
}
if (!isDeletedOrOrphanedNode(traversedState,
greatestExistingAncestorOrSelf, phases, doc)) {
+
// here the node is not orphaned which means that we can reach
the node from root
-switch(fullGcMode) {
-case NONE : {
+switch (fullGcMode) {
+case NONE: {
Review Comment:
still, this in unrelated and we should not increase PR change surface when
that is not necessary or part of the PR
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##
@@ -1226,7 +1260,10 @@ private boolean isDeletedOrOrphanedNode(final NodeState
traversedState, final Pa
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] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
t-rana commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650371659
##
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/fixture/DataStoreUtils.java:
##
@@ -146,26 +150,52 @@ public static void deleteBucket(String bucket,
Map map, Date date) th
}
public static void deleteAzureContainer(Map config, String
containerName) throws Exception {
+if (config == null) {
+return;
Review Comment:
thanks for pointing this out, changes done!
--
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-10742 : add support for defining include/exclude paths for fullGC [jackrabbit-oak]
stefan-egli commented on code in PR #1547:
URL: https://github.com/apache/jackrabbit-oak/pull/1547#discussion_r1650518213
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoVersionGCSupport.java:
##
@@ -132,6 +133,56 @@ public CloseableIterable
getPossiblyDeletedDocs(final long fromMod
input -> store.convertFromDBObject(NODES, input)));
}
+/**
+ * Calculate the bson representing including only the provided
+ * include path prefixes and/or excluding the provided
+ * exclude path prefixes - if any are provided - AND the provided
+ * query.
+ * Please note that at the moment the includes do not
+ * take long paths into account. That is, if a long path was
+ * supposed to be included via an include, it is not.
+ * Reason for this is that long paths would require
+ * the mongo query to include a '_path' condition - which disallows
+ * mongo from using the '_modified_id' index. IOW long paths
+ * would result in full scans - which results in bad performance.
+ * @param includes set of path prefixes which should only be considered
+ * @param excludes set of path prefixes which should be excluded.
+ * if these overlap with includes, then exclude has precedence.
+ * @param query the query with which to do an AND
+ * @return the combined bson with include/exclude path prefixes
+ * AND the provided query
+ */
+private Bson withIncludeExcludes(Set includes, Set
excludes, Bson query) {
+Bson inclExcl = null;
+if (includes != null && !includes.isEmpty()) {
+final List ors = new ArrayList<>(includes.size());
+for (String incl : includes) {
+ors.add(Filters.regex(ID, ":" + incl));
+}
+inclExcl = or(ors);
+}
+if (excludes != null && !excludes.isEmpty()) {
+final List ands = new ArrayList<>(excludes.size());
+for (String excl : excludes) {
+ands.add(not(Filters.regex(ID, ":" + excl)));
+}
Review Comment:
Great, thanks for the heads-up @nfsantos ! I'll need to see how to fix this
(worst case would I guess be to disallow exclude paths until that's improved)
--
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
[PR] OAK-10913 SQL-2 grammar: remove documentation for distinct [jackrabbit-oak]
thomasmueller opened a new pull request, #1552: URL: https://github.com/apache/jackrabbit-oak/pull/1552 (no comment) -- 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-10896: - Add feature toggle for removal of deleted properties and orphaned nodes [jackrabbit-oak]
shodaaan commented on code in PR #1539:
URL: https://github.com/apache/jackrabbit-oak/pull/1539#discussion_r1650451430
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/VersionGCTest.java:
##
@@ -531,6 +531,51 @@ public void testResetFullGCDryRunMode() throws Exception {
// OAK-10370 END
+// OAK-10896
+
+@Test
+public void testVersionGCLoadGCModeConfigurationNotApplicable() throws
Exception {
+int FULL_GC_MODE_NOT_ALLOWED_VALUE = 5;
Review Comment:
Fixed, thank you for the comment
--
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
Re: [PR] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
t-rana commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650374514
##
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/fixture/DataStoreUtilsTest.java:
##
@@ -0,0 +1,259 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.fixture;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.Appender;
+import ch.qos.logback.core.read.ListAppender;
+import com.microsoft.azure.storage.StorageException;
+import com.microsoft.azure.storage.blob.CloudBlobContainer;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPermissions;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPolicy;
+import org.apache.commons.lang3.StringUtils;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureBlobContainerProvider;
+import org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureConstants;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzuriteDockerRule;
+import org.jetbrains.annotations.NotNull;
+import org.junit.After;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.slf4j.LoggerFactory;
+
+import java.net.URISyntaxException;
+import java.security.InvalidKeyException;
+import java.time.Instant;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Date;
+import java.util.EnumSet;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DataStoreUtilsTest {
+@ClassRule
+public static AzuriteDockerRule azuriteDockerRule = new
AzuriteDockerRule();
+
+private static final String AZURE_ACCOUNT_NAME = "AZURE_ACCOUNT_NAME";
+private static final String AZURE_TENANT_ID = "AZURE_TENANT_ID";
+private static final String AZURE_CLIENT_ID = "AZURE_CLIENT_ID";
+private static final String AZURE_CLIENT_SECRET = "AZURE_CLIENT_SECRET";
+private static final String AZURE_CONNECTION_STRING =
"DefaultEndpointsProtocol=http;AccountName=%s;AccountKey=%s;BlobEndpoint=%s";
+private static final String CONTAINER_NAME = "oak-blob-test";
+private static final String AUTHENTICATE_VIA_AZURE_CONNECTION_STRING_LOG =
"connecting to azure blob storage via azureConnectionString";
+private static final String AUTHENTICATE_VIA_ACCESS_KEY_LOG = "connecting
to azure blob storage via access key";
+private static final String AUTHENTICATE_VIA_SERVICE_PRINCIPALS_LOG =
"connecting to azure blob storage via service principal credentials";
+private static final String AUTHENTICATE_VIA_SAS_TOKEN_LOG = "connecting
to azure blob storage via sas token";
+private static final String REFRESH_TOKEN_EXECUTOR_SHUTDOWN_LOG = "Refresh
token executor service shutdown completed";
+private static final String CONTAINER_DOES_NOT_EXIST_MESSAGE = "container
[%s] doesn't exists";
+private static final String CONTAINER_DELETED_MESSAGE = "container [%s]
deleted";
+
+private CloudBlobContainer container;
+
+@Before
+public void init() throws URISyntaxException, InvalidKeyException,
StorageException {
+container = azuriteDockerRule.getContainer(CONTAINER_NAME);
+}
+
+@After
+public void cleanup() throws StorageException {
+if (container != null) {
+container.deleteIfExists();
+}
+}
+
+@Test
+public void delete_non_existing_container_azure_connection_string() throws
Exception {
+container.deleteIfExists();
+ListAppender logAppender = subscribeAppender();
+final String azureConnectionString =
String.format(AZURE_CONNECTION_STRING, AzuriteDockerRule.ACCOUNT_NAME,
AzuriteDockerRule.ACCOUNT_KEY, azuriteDockerRule.getBlobEndpoint());
+
+
DataStoreUtils.deleteAzureContainer(getConfigMap(azureConnectionString, null,
null, null, null, null, null, null), CONTAINER_NAME);
+
Re: [PR] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
t-rana commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650374514
##
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/fixture/DataStoreUtilsTest.java:
##
@@ -0,0 +1,259 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.fixture;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.Appender;
+import ch.qos.logback.core.read.ListAppender;
+import com.microsoft.azure.storage.StorageException;
+import com.microsoft.azure.storage.blob.CloudBlobContainer;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPermissions;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPolicy;
+import org.apache.commons.lang3.StringUtils;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureBlobContainerProvider;
+import org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureConstants;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzuriteDockerRule;
+import org.jetbrains.annotations.NotNull;
+import org.junit.After;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.slf4j.LoggerFactory;
+
+import java.net.URISyntaxException;
+import java.security.InvalidKeyException;
+import java.time.Instant;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Date;
+import java.util.EnumSet;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DataStoreUtilsTest {
+@ClassRule
+public static AzuriteDockerRule azuriteDockerRule = new
AzuriteDockerRule();
+
+private static final String AZURE_ACCOUNT_NAME = "AZURE_ACCOUNT_NAME";
+private static final String AZURE_TENANT_ID = "AZURE_TENANT_ID";
+private static final String AZURE_CLIENT_ID = "AZURE_CLIENT_ID";
+private static final String AZURE_CLIENT_SECRET = "AZURE_CLIENT_SECRET";
+private static final String AZURE_CONNECTION_STRING =
"DefaultEndpointsProtocol=http;AccountName=%s;AccountKey=%s;BlobEndpoint=%s";
+private static final String CONTAINER_NAME = "oak-blob-test";
+private static final String AUTHENTICATE_VIA_AZURE_CONNECTION_STRING_LOG =
"connecting to azure blob storage via azureConnectionString";
+private static final String AUTHENTICATE_VIA_ACCESS_KEY_LOG = "connecting
to azure blob storage via access key";
+private static final String AUTHENTICATE_VIA_SERVICE_PRINCIPALS_LOG =
"connecting to azure blob storage via service principal credentials";
+private static final String AUTHENTICATE_VIA_SAS_TOKEN_LOG = "connecting
to azure blob storage via sas token";
+private static final String REFRESH_TOKEN_EXECUTOR_SHUTDOWN_LOG = "Refresh
token executor service shutdown completed";
+private static final String CONTAINER_DOES_NOT_EXIST_MESSAGE = "container
[%s] doesn't exists";
+private static final String CONTAINER_DELETED_MESSAGE = "container [%s]
deleted";
+
+private CloudBlobContainer container;
+
+@Before
+public void init() throws URISyntaxException, InvalidKeyException,
StorageException {
+container = azuriteDockerRule.getContainer(CONTAINER_NAME);
+}
+
+@After
+public void cleanup() throws StorageException {
+if (container != null) {
+container.deleteIfExists();
+}
+}
+
+@Test
+public void delete_non_existing_container_azure_connection_string() throws
Exception {
+container.deleteIfExists();
+ListAppender logAppender = subscribeAppender();
+final String azureConnectionString =
String.format(AZURE_CONNECTION_STRING, AzuriteDockerRule.ACCOUNT_NAME,
AzuriteDockerRule.ACCOUNT_KEY, azuriteDockerRule.getBlobEndpoint());
+
+
DataStoreUtils.deleteAzureContainer(getConfigMap(azureConnectionString, null,
null, null, null, null, null, null), CONTAINER_NAME);
+
Re: [PR] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
t-rana commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650372569
##
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/fixture/DataStoreUtilsTest.java:
##
@@ -0,0 +1,259 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.fixture;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.Appender;
+import ch.qos.logback.core.read.ListAppender;
+import com.microsoft.azure.storage.StorageException;
+import com.microsoft.azure.storage.blob.CloudBlobContainer;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPermissions;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPolicy;
+import org.apache.commons.lang3.StringUtils;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureBlobContainerProvider;
+import org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureConstants;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzuriteDockerRule;
+import org.jetbrains.annotations.NotNull;
+import org.junit.After;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.slf4j.LoggerFactory;
+
+import java.net.URISyntaxException;
+import java.security.InvalidKeyException;
+import java.time.Instant;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Date;
+import java.util.EnumSet;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DataStoreUtilsTest {
+@ClassRule
+public static AzuriteDockerRule azuriteDockerRule = new
AzuriteDockerRule();
+
+private static final String AZURE_ACCOUNT_NAME = "AZURE_ACCOUNT_NAME";
+private static final String AZURE_TENANT_ID = "AZURE_TENANT_ID";
+private static final String AZURE_CLIENT_ID = "AZURE_CLIENT_ID";
+private static final String AZURE_CLIENT_SECRET = "AZURE_CLIENT_SECRET";
+private static final String AZURE_CONNECTION_STRING =
"DefaultEndpointsProtocol=http;AccountName=%s;AccountKey=%s;BlobEndpoint=%s";
+private static final String CONTAINER_NAME = "oak-blob-test";
+private static final String AUTHENTICATE_VIA_AZURE_CONNECTION_STRING_LOG =
"connecting to azure blob storage via azureConnectionString";
+private static final String AUTHENTICATE_VIA_ACCESS_KEY_LOG = "connecting
to azure blob storage via access key";
+private static final String AUTHENTICATE_VIA_SERVICE_PRINCIPALS_LOG =
"connecting to azure blob storage via service principal credentials";
+private static final String AUTHENTICATE_VIA_SAS_TOKEN_LOG = "connecting
to azure blob storage via sas token";
+private static final String REFRESH_TOKEN_EXECUTOR_SHUTDOWN_LOG = "Refresh
token executor service shutdown completed";
+private static final String CONTAINER_DOES_NOT_EXIST_MESSAGE = "container
[%s] doesn't exists";
+private static final String CONTAINER_DELETED_MESSAGE = "container [%s]
deleted";
+
+private CloudBlobContainer container;
+
+@Before
+public void init() throws URISyntaxException, InvalidKeyException,
StorageException {
+container = azuriteDockerRule.getContainer(CONTAINER_NAME);
+}
+
+@After
+public void cleanup() throws StorageException {
+if (container != null) {
+container.deleteIfExists();
+}
+}
+
+@Test
+public void delete_non_existing_container_azure_connection_string() throws
Exception {
+container.deleteIfExists();
+ListAppender logAppender = subscribeAppender();
+final String azureConnectionString =
String.format(AZURE_CONNECTION_STRING, AzuriteDockerRule.ACCOUNT_NAME,
AzuriteDockerRule.ACCOUNT_KEY, azuriteDockerRule.getBlobEndpoint());
+
+
DataStoreUtils.deleteAzureContainer(getConfigMap(azureConnectionString, null,
null, null, null, null, null, null), CONTAINER_NAME);
+
Re: [PR] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
t-rana commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650372442
##
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/fixture/DataStoreUtilsTest.java:
##
@@ -0,0 +1,259 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.fixture;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.Appender;
+import ch.qos.logback.core.read.ListAppender;
+import com.microsoft.azure.storage.StorageException;
+import com.microsoft.azure.storage.blob.CloudBlobContainer;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPermissions;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPolicy;
+import org.apache.commons.lang3.StringUtils;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureBlobContainerProvider;
+import org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureConstants;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzuriteDockerRule;
+import org.jetbrains.annotations.NotNull;
+import org.junit.After;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.slf4j.LoggerFactory;
+
+import java.net.URISyntaxException;
+import java.security.InvalidKeyException;
+import java.time.Instant;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Date;
+import java.util.EnumSet;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DataStoreUtilsTest {
+@ClassRule
+public static AzuriteDockerRule azuriteDockerRule = new
AzuriteDockerRule();
+
+private static final String AZURE_ACCOUNT_NAME = "AZURE_ACCOUNT_NAME";
+private static final String AZURE_TENANT_ID = "AZURE_TENANT_ID";
+private static final String AZURE_CLIENT_ID = "AZURE_CLIENT_ID";
+private static final String AZURE_CLIENT_SECRET = "AZURE_CLIENT_SECRET";
+private static final String AZURE_CONNECTION_STRING =
"DefaultEndpointsProtocol=http;AccountName=%s;AccountKey=%s;BlobEndpoint=%s";
+private static final String CONTAINER_NAME = "oak-blob-test";
+private static final String AUTHENTICATE_VIA_AZURE_CONNECTION_STRING_LOG =
"connecting to azure blob storage via azureConnectionString";
+private static final String AUTHENTICATE_VIA_ACCESS_KEY_LOG = "connecting
to azure blob storage via access key";
+private static final String AUTHENTICATE_VIA_SERVICE_PRINCIPALS_LOG =
"connecting to azure blob storage via service principal credentials";
+private static final String AUTHENTICATE_VIA_SAS_TOKEN_LOG = "connecting
to azure blob storage via sas token";
+private static final String REFRESH_TOKEN_EXECUTOR_SHUTDOWN_LOG = "Refresh
token executor service shutdown completed";
+private static final String CONTAINER_DOES_NOT_EXIST_MESSAGE = "container
[%s] doesn't exists";
+private static final String CONTAINER_DELETED_MESSAGE = "container [%s]
deleted";
+
+private CloudBlobContainer container;
+
+@Before
+public void init() throws URISyntaxException, InvalidKeyException,
StorageException {
+container = azuriteDockerRule.getContainer(CONTAINER_NAME);
+}
+
+@After
+public void cleanup() throws StorageException {
+if (container != null) {
+container.deleteIfExists();
+}
+}
+
+@Test
+public void delete_non_existing_container_azure_connection_string() throws
Exception {
+container.deleteIfExists();
Review Comment:
now using a random container name while trying to delete container that are
never created.
--
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:
Re: [PR] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
t-rana commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650372015
##
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/fixture/DataStoreUtilsTest.java:
##
@@ -0,0 +1,259 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.fixture;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.Appender;
+import ch.qos.logback.core.read.ListAppender;
+import com.microsoft.azure.storage.StorageException;
+import com.microsoft.azure.storage.blob.CloudBlobContainer;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPermissions;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPolicy;
+import org.apache.commons.lang3.StringUtils;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureBlobContainerProvider;
+import org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureConstants;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzuriteDockerRule;
+import org.jetbrains.annotations.NotNull;
+import org.junit.After;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.slf4j.LoggerFactory;
+
+import java.net.URISyntaxException;
+import java.security.InvalidKeyException;
+import java.time.Instant;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Date;
+import java.util.EnumSet;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DataStoreUtilsTest {
+@ClassRule
+public static AzuriteDockerRule azuriteDockerRule = new
AzuriteDockerRule();
+
+private static final String AZURE_ACCOUNT_NAME = "AZURE_ACCOUNT_NAME";
+private static final String AZURE_TENANT_ID = "AZURE_TENANT_ID";
+private static final String AZURE_CLIENT_ID = "AZURE_CLIENT_ID";
+private static final String AZURE_CLIENT_SECRET = "AZURE_CLIENT_SECRET";
+private static final String AZURE_CONNECTION_STRING =
"DefaultEndpointsProtocol=http;AccountName=%s;AccountKey=%s;BlobEndpoint=%s";
+private static final String CONTAINER_NAME = "oak-blob-test";
+private static final String AUTHENTICATE_VIA_AZURE_CONNECTION_STRING_LOG =
"connecting to azure blob storage via azureConnectionString";
+private static final String AUTHENTICATE_VIA_ACCESS_KEY_LOG = "connecting
to azure blob storage via access key";
+private static final String AUTHENTICATE_VIA_SERVICE_PRINCIPALS_LOG =
"connecting to azure blob storage via service principal credentials";
+private static final String AUTHENTICATE_VIA_SAS_TOKEN_LOG = "connecting
to azure blob storage via sas token";
+private static final String REFRESH_TOKEN_EXECUTOR_SHUTDOWN_LOG = "Refresh
token executor service shutdown completed";
+private static final String CONTAINER_DOES_NOT_EXIST_MESSAGE = "container
[%s] doesn't exists";
+private static final String CONTAINER_DELETED_MESSAGE = "container [%s]
deleted";
+
+private CloudBlobContainer container;
+
+@Before
+public void init() throws URISyntaxException, InvalidKeyException,
StorageException {
+container = azuriteDockerRule.getContainer(CONTAINER_NAME);
Review Comment:
added a assert for container existence
--
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] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
t-rana commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650371659
##
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/fixture/DataStoreUtils.java:
##
@@ -146,26 +150,52 @@ public static void deleteBucket(String bucket,
Map map, Date date) th
}
public static void deleteAzureContainer(Map config, String
containerName) throws Exception {
+if (config == null) {
+return;
Review Comment:
thanks for pointing this our, changes done!
##
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/fixture/DataStoreUtils.java:
##
@@ -146,26 +150,52 @@ public static void deleteBucket(String bucket,
Map map, Date date) th
}
public static void deleteAzureContainer(Map config, String
containerName) throws Exception {
+if (config == null) {
+return;
+}
if (Strings.isNullOrEmpty(containerName)) {
return;
Review Comment:
thanks, changes done!
--
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] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
t-rana commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650371492
##
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/fixture/DataStoreUtils.java:
##
@@ -146,26 +150,52 @@ public static void deleteBucket(String bucket,
Map map, Date date) th
}
public static void deleteAzureContainer(Map config, String
containerName) throws Exception {
+if (config == null) {
+return;
+}
if (Strings.isNullOrEmpty(containerName)) {
return;
}
-String connectionString = (String)
config.get(AzureConstants.AZURE_CONNECTION_STRING);
-if (Strings.isNullOrEmpty(connectionString)) {
-String accountName = (String)
config.get(AzureConstants.AZURE_STORAGE_ACCOUNT_NAME);
-String accountKey = (String)
config.get(AzureConstants.AZURE_STORAGE_ACCOUNT_KEY);
-if (Strings.isNullOrEmpty(accountName) ||
-Strings.isNullOrEmpty(accountKey)) {
-return;
-}
-connectionString =
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.Utils.getConnectionString(accountName,
accountKey);
+CloudBlobContainer container = getCloudBlobContainer(config,
containerName);
+if (container == null) {
+log.info("container is not initialized");
Review Comment:
done
--
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] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
nit0906 commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650350025
##
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/fixture/DataStoreUtils.java:
##
@@ -146,26 +150,52 @@ public static void deleteBucket(String bucket,
Map map, Date date) th
}
public static void deleteAzureContainer(Map config, String
containerName) throws Exception {
+if (config == null) {
+return;
+}
if (Strings.isNullOrEmpty(containerName)) {
return;
}
-String connectionString = (String)
config.get(AzureConstants.AZURE_CONNECTION_STRING);
-if (Strings.isNullOrEmpty(connectionString)) {
-String accountName = (String)
config.get(AzureConstants.AZURE_STORAGE_ACCOUNT_NAME);
-String accountKey = (String)
config.get(AzureConstants.AZURE_STORAGE_ACCOUNT_KEY);
-if (Strings.isNullOrEmpty(accountName) ||
-Strings.isNullOrEmpty(accountKey)) {
-return;
-}
-connectionString =
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.Utils.getConnectionString(accountName,
accountKey);
+CloudBlobContainer container = getCloudBlobContainer(config,
containerName);
+if (container == null) {
+log.info("container is not initialized");
Review Comment:
This should be changed to warn.
--
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] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
nit0906 commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650348764
##
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/fixture/DataStoreUtilsTest.java:
##
@@ -0,0 +1,259 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.fixture;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.Appender;
+import ch.qos.logback.core.read.ListAppender;
+import com.microsoft.azure.storage.StorageException;
+import com.microsoft.azure.storage.blob.CloudBlobContainer;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPermissions;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPolicy;
+import org.apache.commons.lang3.StringUtils;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureBlobContainerProvider;
+import org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureConstants;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzuriteDockerRule;
+import org.jetbrains.annotations.NotNull;
+import org.junit.After;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.slf4j.LoggerFactory;
+
+import java.net.URISyntaxException;
+import java.security.InvalidKeyException;
+import java.time.Instant;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Date;
+import java.util.EnumSet;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DataStoreUtilsTest {
+@ClassRule
+public static AzuriteDockerRule azuriteDockerRule = new
AzuriteDockerRule();
+
+private static final String AZURE_ACCOUNT_NAME = "AZURE_ACCOUNT_NAME";
+private static final String AZURE_TENANT_ID = "AZURE_TENANT_ID";
+private static final String AZURE_CLIENT_ID = "AZURE_CLIENT_ID";
+private static final String AZURE_CLIENT_SECRET = "AZURE_CLIENT_SECRET";
+private static final String AZURE_CONNECTION_STRING =
"DefaultEndpointsProtocol=http;AccountName=%s;AccountKey=%s;BlobEndpoint=%s";
+private static final String CONTAINER_NAME = "oak-blob-test";
+private static final String AUTHENTICATE_VIA_AZURE_CONNECTION_STRING_LOG =
"connecting to azure blob storage via azureConnectionString";
+private static final String AUTHENTICATE_VIA_ACCESS_KEY_LOG = "connecting
to azure blob storage via access key";
+private static final String AUTHENTICATE_VIA_SERVICE_PRINCIPALS_LOG =
"connecting to azure blob storage via service principal credentials";
+private static final String AUTHENTICATE_VIA_SAS_TOKEN_LOG = "connecting
to azure blob storage via sas token";
+private static final String REFRESH_TOKEN_EXECUTOR_SHUTDOWN_LOG = "Refresh
token executor service shutdown completed";
+private static final String CONTAINER_DOES_NOT_EXIST_MESSAGE = "container
[%s] doesn't exists";
+private static final String CONTAINER_DELETED_MESSAGE = "container [%s]
deleted";
+
+private CloudBlobContainer container;
+
+@Before
+public void init() throws URISyntaxException, InvalidKeyException,
StorageException {
+container = azuriteDockerRule.getContainer(CONTAINER_NAME);
+}
+
+@After
+public void cleanup() throws StorageException {
+if (container != null) {
+container.deleteIfExists();
+}
+}
+
+@Test
+public void delete_non_existing_container_azure_connection_string() throws
Exception {
+container.deleteIfExists();
+ListAppender logAppender = subscribeAppender();
+final String azureConnectionString =
String.format(AZURE_CONNECTION_STRING, AzuriteDockerRule.ACCOUNT_NAME,
AzuriteDockerRule.ACCOUNT_KEY, azuriteDockerRule.getBlobEndpoint());
+
+
DataStoreUtils.deleteAzureContainer(getConfigMap(azureConnectionString, null,
null, null, null, null, null, null), CONTAINER_NAME);
+
Re: [PR] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
nit0906 commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650348034
##
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/fixture/DataStoreUtilsTest.java:
##
@@ -0,0 +1,259 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.fixture;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.Appender;
+import ch.qos.logback.core.read.ListAppender;
+import com.microsoft.azure.storage.StorageException;
+import com.microsoft.azure.storage.blob.CloudBlobContainer;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPermissions;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPolicy;
+import org.apache.commons.lang3.StringUtils;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureBlobContainerProvider;
+import org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureConstants;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzuriteDockerRule;
+import org.jetbrains.annotations.NotNull;
+import org.junit.After;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.slf4j.LoggerFactory;
+
+import java.net.URISyntaxException;
+import java.security.InvalidKeyException;
+import java.time.Instant;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Date;
+import java.util.EnumSet;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DataStoreUtilsTest {
+@ClassRule
+public static AzuriteDockerRule azuriteDockerRule = new
AzuriteDockerRule();
+
+private static final String AZURE_ACCOUNT_NAME = "AZURE_ACCOUNT_NAME";
+private static final String AZURE_TENANT_ID = "AZURE_TENANT_ID";
+private static final String AZURE_CLIENT_ID = "AZURE_CLIENT_ID";
+private static final String AZURE_CLIENT_SECRET = "AZURE_CLIENT_SECRET";
+private static final String AZURE_CONNECTION_STRING =
"DefaultEndpointsProtocol=http;AccountName=%s;AccountKey=%s;BlobEndpoint=%s";
+private static final String CONTAINER_NAME = "oak-blob-test";
+private static final String AUTHENTICATE_VIA_AZURE_CONNECTION_STRING_LOG =
"connecting to azure blob storage via azureConnectionString";
+private static final String AUTHENTICATE_VIA_ACCESS_KEY_LOG = "connecting
to azure blob storage via access key";
+private static final String AUTHENTICATE_VIA_SERVICE_PRINCIPALS_LOG =
"connecting to azure blob storage via service principal credentials";
+private static final String AUTHENTICATE_VIA_SAS_TOKEN_LOG = "connecting
to azure blob storage via sas token";
+private static final String REFRESH_TOKEN_EXECUTOR_SHUTDOWN_LOG = "Refresh
token executor service shutdown completed";
+private static final String CONTAINER_DOES_NOT_EXIST_MESSAGE = "container
[%s] doesn't exists";
+private static final String CONTAINER_DELETED_MESSAGE = "container [%s]
deleted";
+
+private CloudBlobContainer container;
+
+@Before
+public void init() throws URISyntaxException, InvalidKeyException,
StorageException {
+container = azuriteDockerRule.getContainer(CONTAINER_NAME);
+}
+
+@After
+public void cleanup() throws StorageException {
+if (container != null) {
+container.deleteIfExists();
+}
+}
+
+@Test
+public void delete_non_existing_container_azure_connection_string() throws
Exception {
+container.deleteIfExists();
Review Comment:
instead of deleting the container here - couldn't we simply use a random
container name that we never created ?
--
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
Re: [PR] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
nit0906 commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650346400
##
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/fixture/DataStoreUtilsTest.java:
##
@@ -0,0 +1,259 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.fixture;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.Appender;
+import ch.qos.logback.core.read.ListAppender;
+import com.microsoft.azure.storage.StorageException;
+import com.microsoft.azure.storage.blob.CloudBlobContainer;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPermissions;
+import com.microsoft.azure.storage.blob.SharedAccessBlobPolicy;
+import org.apache.commons.lang3.StringUtils;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureBlobContainerProvider;
+import org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzureConstants;
+import
org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzuriteDockerRule;
+import org.jetbrains.annotations.NotNull;
+import org.junit.After;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.slf4j.LoggerFactory;
+
+import java.net.URISyntaxException;
+import java.security.InvalidKeyException;
+import java.time.Instant;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Date;
+import java.util.EnumSet;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DataStoreUtilsTest {
+@ClassRule
+public static AzuriteDockerRule azuriteDockerRule = new
AzuriteDockerRule();
+
+private static final String AZURE_ACCOUNT_NAME = "AZURE_ACCOUNT_NAME";
+private static final String AZURE_TENANT_ID = "AZURE_TENANT_ID";
+private static final String AZURE_CLIENT_ID = "AZURE_CLIENT_ID";
+private static final String AZURE_CLIENT_SECRET = "AZURE_CLIENT_SECRET";
+private static final String AZURE_CONNECTION_STRING =
"DefaultEndpointsProtocol=http;AccountName=%s;AccountKey=%s;BlobEndpoint=%s";
+private static final String CONTAINER_NAME = "oak-blob-test";
+private static final String AUTHENTICATE_VIA_AZURE_CONNECTION_STRING_LOG =
"connecting to azure blob storage via azureConnectionString";
+private static final String AUTHENTICATE_VIA_ACCESS_KEY_LOG = "connecting
to azure blob storage via access key";
+private static final String AUTHENTICATE_VIA_SERVICE_PRINCIPALS_LOG =
"connecting to azure blob storage via service principal credentials";
+private static final String AUTHENTICATE_VIA_SAS_TOKEN_LOG = "connecting
to azure blob storage via sas token";
+private static final String REFRESH_TOKEN_EXECUTOR_SHUTDOWN_LOG = "Refresh
token executor service shutdown completed";
+private static final String CONTAINER_DOES_NOT_EXIST_MESSAGE = "container
[%s] doesn't exists";
+private static final String CONTAINER_DELETED_MESSAGE = "container [%s]
deleted";
+
+private CloudBlobContainer container;
+
+@Before
+public void init() throws URISyntaxException, InvalidKeyException,
StorageException {
+container = azuriteDockerRule.getContainer(CONTAINER_NAME);
Review Comment:
Does this init method assures that the container will always be present
before any test executes ?
--
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] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
nit0906 commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650340220
##
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/fixture/DataStoreUtils.java:
##
@@ -146,26 +150,52 @@ public static void deleteBucket(String bucket,
Map map, Date date) th
}
public static void deleteAzureContainer(Map config, String
containerName) throws Exception {
+if (config == null) {
+return;
Review Comment:
cannot initialize*
--
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] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
nit0906 commented on code in PR #1542:
URL: https://github.com/apache/jackrabbit-oak/pull/1542#discussion_r1650340383
##
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/fixture/DataStoreUtils.java:
##
@@ -146,26 +150,52 @@ public static void deleteBucket(String bucket,
Map map, Date date) th
}
public static void deleteAzureContainer(Map config, String
containerName) throws Exception {
+if (config == null) {
+return;
+}
if (Strings.isNullOrEmpty(containerName)) {
return;
Review Comment:
cannot initialize*
--
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-10904: change token refresh executor to use daemon thread [jackrabbit-oak]
t-rana commented on code in PR #1545:
URL: https://github.com/apache/jackrabbit-oak/pull/1545#discussion_r1648425265
##
oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureUtilities.java:
##
@@ -74,7 +74,12 @@ public final class AzureUtilities {
private static final long TOKEN_REFRESHER_DELAY = 1L;
private static final Logger log =
LoggerFactory.getLogger(AzureUtilities.class);
-private static final ScheduledExecutorService executorService =
Executors.newSingleThreadScheduledExecutor();
+
+private static final ScheduledExecutorService executorService =
Executors.newSingleThreadScheduledExecutor(runnable -> {
Review Comment:
> Not closing it properly could cause random issues in tests for example
I did not face any issues in the test suite run, can you please elaborate so
that I can handle the same? I guess the framework running test terminates the
JVM and shutdown method is executed which will close this exectuor.
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
stefan-egli commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644787553
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -60,6 +73,11 @@ public void before() throws Exception {
ns = builderProvider.newBuilder().setBlobStore(bs).getNodeStore();
}
+@After
+public void tearDown() {
+ns.dispose();
Review Comment:
```suggestion
try {
ns.dispose();
} finally {
DocumentPropertyState.setCompressionThreshold(DISABLED_COMPRESSION);
}
```
to avoid side-effects, the test should ensure it doesn't leak a non default
compression threshold to other tests. DISABLED_COMPRESSION is for illustration
to refer to -1 - ideally the -1 would only be defined in the source class...
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
stefan-egli commented on PR #1526: URL: https://github.com/apache/jackrabbit-oak/pull/1526#issuecomment-2176272288 [looks](https://github.com/apache/jackrabbit-oak/blob/14c357120e49d83281dc473d810ca485deb94501/oak-store-spi/src/main/java/org/apache/jackrabbit/oak/plugins/memory/AbstractPropertyState.java#L103-L106) like the original intention was that properties shouldn't or wouldn't be used as keys in hash maps... if that's the case then it should be fine.. -- 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] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1644584397
##
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/MoveVersionableNodeRepositoryTest.java:
##
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.jcr;
+
+import org.apache.jackrabbit.JcrConstants;
+import org.apache.jackrabbit.oak.NodeStoreFixtures;
+import org.apache.jackrabbit.oak.fixture.NodeStoreFixture;
+import org.apache.jackrabbit.test.NotExecutableException;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+import javax.jcr.Node;
+import javax.jcr.Session;
+import javax.jcr.version.Version;
+import javax.jcr.version.VersionHistory;
+import javax.jcr.version.VersionIterator;
+import javax.jcr.version.VersionManager;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import static java.util.Collections.singleton;
+import static
org.apache.jackrabbit.oak.commons.FixturesHelper.Fixture.DOCUMENT_NS;
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Test for moving versionable nodes over deleted versionable nodes using the
DOCUMENT_NS fixture.
+ */
+public class MoveVersionableNodeRepositoryTest extends AbstractRepositoryTest {
+
+public MoveVersionableNodeRepositoryTest(NodeStoreFixture fixture) {
+super(fixture);
+}
+
+@Parameterized.Parameters(name="{0}")
+public static Collection memoryFixture() {
+return NodeStoreFixtures.asJunitParameters(singleton(DOCUMENT_NS));
+}
+
+/**
+ * Creates a versionable node with the specified name under the given
parent node, then
+ * saves the session.
+ * @param parent
+ * @param nodeName
+ * @return
+ * @throws Exception
+ */
+private Node createVersionableNode(Node parent, String nodeName) throws
Exception {
+Node newNode = (parent.hasNode(nodeName)) ? parent.getNode(nodeName) :
parent.addNode(nodeName);
+if (newNode.canAddMixin(JcrConstants.MIX_VERSIONABLE)) {
+newNode.addMixin(JcrConstants.MIX_VERSIONABLE);
+} else {
+throw new NotExecutableException();
+}
+newNode.getSession().save();
+return newNode;
+}
+
+/**
+ * Checks out the node, sets the property then saves the session and
checks the node back in.
+ * To be used in tests where version history needs to be populated.
+ */
+private void setNodePropertyAndCheckIn(Node node, String propertyName,
String propertyValue) throws Exception {
+node.checkout();
+node.setProperty(propertyName, propertyValue);
+node.getSession().save();
+node.checkin();
+}
+
+/*
+ * 1. Create a versionable unstructured node at
nodeName1/nodeName2/sourceNode
+ * 2. Create a versionable unstructured node at
nodeName1/nodeName3/sourceNode
+ * 3. create version histories for both nodes
+ * 4. remove nodeName1/nodeName3/nodeName1 (that's because move(src,dest)
throws an exception if dest already exists)
+ * 5. move nodeName1/nodeName2/sourceNode to
nodeName1/nodeName3/sourceNode and call session.save()
+ * 6. should work according to JCR specification - reproduces bug for
OAK-8848: will throw ConstraintViolationException
+ * - "Property is protected: jcr:versionHistory
+ */
+@Test
+public void
testMoveNodeWithVersionHistoryOverDeletedNodeWithVersionHistory() throws
Exception {
+
+Session session = getAdminSession();
+Node testRootNode = session.getRootNode().addNode("node1");
+
+String newNodeName = "sourceNode";
+Node sourceParent = testRootNode.addNode("node2"); //
nodeName1/nodeName2
+Node sourceNode = createVersionableNode(sourceParent, newNodeName); //
nodeName1/nodeName2/sourceNode
+Node destParent = testRootNode.addNode("node3"); // nodeName1/nodeName3
+Node destNode = createVersionableNode(destParent, "destNode"); //
nodeName1/nodeName3/sourceNode
+
+String destPath = destNode.getPath();
+
+// add version histories for sourceNode and destNode
+setNodePropertyAndCheckIn(sourceNode,
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
stefan-egli commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1644565491
##
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/MoveVersionableNodeRepositoryTest.java:
##
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.jcr;
+
+import org.apache.jackrabbit.JcrConstants;
+import org.apache.jackrabbit.oak.NodeStoreFixtures;
+import org.apache.jackrabbit.oak.fixture.NodeStoreFixture;
+import org.apache.jackrabbit.test.NotExecutableException;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+import javax.jcr.Node;
+import javax.jcr.Session;
+import javax.jcr.version.Version;
+import javax.jcr.version.VersionHistory;
+import javax.jcr.version.VersionIterator;
+import javax.jcr.version.VersionManager;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import static java.util.Collections.singleton;
+import static
org.apache.jackrabbit.oak.commons.FixturesHelper.Fixture.DOCUMENT_NS;
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Test for moving versionable nodes over deleted versionable nodes using the
DOCUMENT_NS fixture.
+ */
+public class MoveVersionableNodeRepositoryTest extends AbstractRepositoryTest {
+
+public MoveVersionableNodeRepositoryTest(NodeStoreFixture fixture) {
+super(fixture);
+}
+
+@Parameterized.Parameters(name="{0}")
+public static Collection memoryFixture() {
+return NodeStoreFixtures.asJunitParameters(singleton(DOCUMENT_NS));
+}
+
+/**
+ * Creates a versionable node with the specified name under the given
parent node, then
+ * saves the session.
+ * @param parent
+ * @param nodeName
+ * @return
+ * @throws Exception
+ */
+private Node createVersionableNode(Node parent, String nodeName) throws
Exception {
+Node newNode = (parent.hasNode(nodeName)) ? parent.getNode(nodeName) :
parent.addNode(nodeName);
+if (newNode.canAddMixin(JcrConstants.MIX_VERSIONABLE)) {
+newNode.addMixin(JcrConstants.MIX_VERSIONABLE);
+} else {
+throw new NotExecutableException();
+}
+newNode.getSession().save();
+return newNode;
+}
+
+/**
+ * Checks out the node, sets the property then saves the session and
checks the node back in.
+ * To be used in tests where version history needs to be populated.
+ */
+private void setNodePropertyAndCheckIn(Node node, String propertyName,
String propertyValue) throws Exception {
+node.checkout();
+node.setProperty(propertyName, propertyValue);
+node.getSession().save();
+node.checkin();
+}
+
+/*
+ * 1. Create a versionable unstructured node at
nodeName1/nodeName2/sourceNode
+ * 2. Create a versionable unstructured node at
nodeName1/nodeName3/sourceNode
+ * 3. create version histories for both nodes
+ * 4. remove nodeName1/nodeName3/nodeName1 (that's because move(src,dest)
throws an exception if dest already exists)
+ * 5. move nodeName1/nodeName2/sourceNode to
nodeName1/nodeName3/sourceNode and call session.save()
+ * 6. should work according to JCR specification - reproduces bug for
OAK-8848: will throw ConstraintViolationException
+ * - "Property is protected: jcr:versionHistory
+ */
+@Test
+public void
testMoveNodeWithVersionHistoryOverDeletedNodeWithVersionHistory() throws
Exception {
+
+Session session = getAdminSession();
+Node testRootNode = session.getRootNode().addNode("node1");
+
+String newNodeName = "sourceNode";
+Node sourceParent = testRootNode.addNode("node2"); //
nodeName1/nodeName2
+Node sourceNode = createVersionableNode(sourceParent, newNodeName); //
nodeName1/nodeName2/sourceNode
+Node destParent = testRootNode.addNode("node3"); // nodeName1/nodeName3
+Node destNode = createVersionableNode(destParent, "destNode"); //
nodeName1/nodeName3/sourceNode
+
+String destPath = destNode.getPath();
+
+// add version histories for sourceNode and destNode
+setNodePropertyAndCheckIn(sourceNode,
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644558711
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -127,8 +188,25 @@ public boolean equals(Object object) {
return true;
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
-return this.name.equals(other.name)
-&& this.value.equals(other.value);
+// Compare names and raw un-parsed values
+if (!this.name.equals(other.name) ||
!this.getValue().equals(other.getValue())) {
Review Comment:
Yes, but only then, not always.
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
reschke commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644549857
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -127,8 +188,25 @@ public boolean equals(Object object) {
return true;
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
-return this.name.equals(other.name)
-&& this.value.equals(other.value);
+// Compare names and raw un-parsed values
+if (!this.name.equals(other.name) ||
!this.getValue().equals(other.getValue())) {
Review Comment:
"getValue()" will uncompress when the value is compressed, no?
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644545109
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -127,8 +188,25 @@ public boolean equals(Object object) {
return true;
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
-return this.name.equals(other.name)
-&& this.value.equals(other.value);
+// Compare names and raw un-parsed values
+if (!this.name.equals(other.name) ||
!this.getValue().equals(other.getValue())) {
Review Comment:
Not necessarily.
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644545109
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -127,8 +188,25 @@ public boolean equals(Object object) {
return true;
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
-return this.name.equals(other.name)
-&& this.value.equals(other.value);
+// Compare names and raw un-parsed values
+if (!this.name.equals(other.name) ||
!this.getValue().equals(other.getValue())) {
Review Comment:
Not necessary.
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
reschke commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644499232
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -127,8 +188,25 @@ public boolean equals(Object object) {
return true;
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
-return this.name.equals(other.name)
-&& this.value.equals(other.value);
+// Compare names and raw un-parsed values
+if (!this.name.equals(other.name) ||
!this.getValue().equals(other.getValue())) {
Review Comment:
This will always decompress, no?
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644492664
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -81,4 +100,225 @@ public void multiValuedBinarySize() throws Exception {
assertEquals(0, reads.size());
}
-}
+@Test
+public void multiValuedAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+List blobs = newArrayList();
+for (int i = 0; i < 13; i++) {
+blobs.add(builder.createBlob(new RandomStream(BLOB_SIZE, i)));
+}
+builder.child(TEST_NODE).setProperty("p", blobs, Type.BINARIES);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.BINARIES, Objects.requireNonNull(p).getType());
+assertEquals(13, p.count());
+
+reads.clear();
+assertEquals(BLOB_SIZE, p.size(0));
+// must not read the blob via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringBelowThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", "dummy", Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(5, p.size(0));
+// must not read the string via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", STRING_HUGEVALUE,
Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(10050, p.size(0));
+// must not read the string via streams
+assertEquals(0, reads.size());
+}
+
+@Test
+public void compressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", "\"" + STRING_HUGEVALUE + "\"",
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING),
STRING_HUGEVALUE);
+
+verify(mockCompression,
times(1)).getOutputStream(any(OutputStream.class));
+
+compressionThreshold.set(null, -1);
+
+}
+
+@Test
+public void uncompressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+OutputStream mockOutputStream= mock(OutputStream.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenReturn(mockOutputStream);
+
when(mockCompression.getInputStream(any(InputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", STRING_HUGEVALUE,
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING), "{}");
+
+verify(mockCompression,
times(1)).getInputStream(any(InputStream.class));
+
+
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644490150
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -81,4 +100,225 @@ public void multiValuedBinarySize() throws Exception {
assertEquals(0, reads.size());
}
-}
+@Test
+public void multiValuedAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+List blobs = newArrayList();
+for (int i = 0; i < 13; i++) {
+blobs.add(builder.createBlob(new RandomStream(BLOB_SIZE, i)));
+}
+builder.child(TEST_NODE).setProperty("p", blobs, Type.BINARIES);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.BINARIES, Objects.requireNonNull(p).getType());
+assertEquals(13, p.count());
+
+reads.clear();
+assertEquals(BLOB_SIZE, p.size(0));
+// must not read the blob via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringBelowThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", "dummy", Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(5, p.size(0));
+// must not read the string via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", STRING_HUGEVALUE,
Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(10050, p.size(0));
+// must not read the string via streams
+assertEquals(0, reads.size());
+}
+
+@Test
+public void compressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", "\"" + STRING_HUGEVALUE + "\"",
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING),
STRING_HUGEVALUE);
+
+verify(mockCompression,
times(1)).getOutputStream(any(OutputStream.class));
+
+compressionThreshold.set(null, -1);
+
Review Comment:
Done
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644489555
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -116,7 +163,20 @@ public int count() {
*/
@NotNull
String getValue() {
-return value;
+return value != null ? value : decompress(this.compressedValue);
+}
+
+private String decompress(byte[] value) {
+try {
+return new String(compression.getInputStream(new
ByteArrayInputStream(value)).readAllBytes(), StandardCharsets.UTF_8);
+} catch (IOException e) {
+LOG.error("Failed to decompress property {} value: ", getName(),
e);
+return "\"{}\"";
+}
+}
+
+private byte[] getCompressedValue() {
Review Comment:
Done
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -81,4 +100,225 @@ public void multiValuedBinarySize() throws Exception {
assertEquals(0, reads.size());
}
-}
+@Test
+public void multiValuedAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+List blobs = newArrayList();
+for (int i = 0; i < 13; i++) {
+blobs.add(builder.createBlob(new RandomStream(BLOB_SIZE, i)));
+}
+builder.child(TEST_NODE).setProperty("p", blobs, Type.BINARIES);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.BINARIES, Objects.requireNonNull(p).getType());
+assertEquals(13, p.count());
+
+reads.clear();
+assertEquals(BLOB_SIZE, p.size(0));
+// must not read the blob via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringBelowThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", "dummy", Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(5, p.size(0));
+// must not read the string via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", STRING_HUGEVALUE,
Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(10050, p.size(0));
+// must not read the string via streams
+assertEquals(0, reads.size());
+}
+
+@Test
+public void compressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
Review Comment:
Done
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644489190
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -18,26 +18,45 @@
import java.io.IOException;
import java.io.InputStream;
+import java.io.OutputStream;
+import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
import java.util.List;
+import java.util.Objects;
import java.util.Set;
+import org.apache.commons.lang3.RandomStringUtils;
import org.apache.jackrabbit.oak.api.Blob;
import org.apache.jackrabbit.oak.api.PropertyState;
import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.commons.Compression;
import org.apache.jackrabbit.oak.spi.blob.BlobStore;
import org.apache.jackrabbit.oak.spi.blob.MemoryBlobStore;
import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
+import org.junit.*;
+import org.junit.runners.MethodSorters;
import static org.apache.jackrabbit.guava.common.collect.Lists.newArrayList;
import static org.apache.jackrabbit.guava.common.collect.Sets.newHashSet;
import static org.junit.Assert.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+@FixMethodOrder(MethodSorters.NAME_ASCENDING)
public class DocumentPropertyStateTest {
private static final int BLOB_SIZE = 16 * 1024;
+public static final int COMPRESSED_SIZE = 543;
Review Comment:
Done
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644488604
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -38,24 +45,64 @@
import org.apache.jackrabbit.oak.plugins.memory.StringPropertyState;
import org.apache.jackrabbit.oak.plugins.value.Conversions;
import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* PropertyState implementation with lazy parsing of the JSOP encoded value.
*/
final class DocumentPropertyState implements PropertyState {
+private static final Logger LOG =
LoggerFactory.getLogger(DocumentPropertyState.class);
+
private final DocumentNodeStore store;
private final String name;
private final String value;
private PropertyState parsed;
+private final byte[] compressedValue;
+private final Compression compression;
+
+private static final int DEFAULT_COMPRESSION_THRESHOLD =
SystemPropertySupplier
+.create("oak.documentMK.stringCompressionThreshold ",
-1).loggingTo(LOG).get();
DocumentPropertyState(DocumentNodeStore store, String name, String value) {
+this(store, name, value, Compression.GZIP);
+}
+
+DocumentPropertyState(DocumentNodeStore store, String name, String value,
Compression compression) {
this.store = store;
this.name = name;
-this.value = value;
+if (compression != null && DEFAULT_COMPRESSION_THRESHOLD == -1) {
Review Comment:
Done
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644487143
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -81,4 +100,179 @@ public void multiValuedBinarySize() throws Exception {
assertEquals(0, reads.size());
}
-}
+@Test
+public void multiValuedAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+List blobs = newArrayList();
+for (int i = 0; i < 13; i++) {
+blobs.add(builder.createBlob(new RandomStream(BLOB_SIZE, i)));
+}
+builder.child(TEST_NODE).setProperty("p", blobs, Type.BINARIES);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.BINARIES, Objects.requireNonNull(p).getType());
+assertEquals(13, p.count());
+
+reads.clear();
+assertEquals(BLOB_SIZE, p.size(0));
+// must not read the blob via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringBelowThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", "dummy", Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(5, p.size(0));
+// must not read the string via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", STRING_HUGEVALUE,
Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(10050, p.size(0));
+// must not read the string via streams
+assertEquals(0, reads.size());
+}
+
+@Test
+public void compressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", "\"" + STRING_HUGEVALUE + "\"",
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING),
STRING_HUGEVALUE);
+
+verify(mockCompression,
times(1)).getOutputStream(any(OutputStream.class));
+
+compressionThreshold.set(null, -1);
+
+}
+
+@Test
+public void uncompressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+OutputStream mockOutputStream= mock(OutputStream.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenReturn(mockOutputStream);
+
when(mockCompression.getInputStream(any(InputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", STRING_HUGEVALUE,
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING), "{}");
+
+verify(mockCompression,
times(1)).getInputStream(any(InputStream.class));
+
+
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644486368
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -81,4 +100,179 @@ public void multiValuedBinarySize() throws Exception {
assertEquals(0, reads.size());
}
-}
+@Test
+public void multiValuedAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+List blobs = newArrayList();
+for (int i = 0; i < 13; i++) {
+blobs.add(builder.createBlob(new RandomStream(BLOB_SIZE, i)));
+}
+builder.child(TEST_NODE).setProperty("p", blobs, Type.BINARIES);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.BINARIES, Objects.requireNonNull(p).getType());
+assertEquals(13, p.count());
+
+reads.clear();
+assertEquals(BLOB_SIZE, p.size(0));
+// must not read the blob via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringBelowThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", "dummy", Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(5, p.size(0));
+// must not read the string via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", STRING_HUGEVALUE,
Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(10050, p.size(0));
+// must not read the string via streams
+assertEquals(0, reads.size());
+}
+
+@Test
+public void compressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", "\"" + STRING_HUGEVALUE + "\"",
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING),
STRING_HUGEVALUE);
+
+verify(mockCompression,
times(1)).getOutputStream(any(OutputStream.class));
+
+compressionThreshold.set(null, -1);
+
+}
+
+@Test
+public void uncompressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+OutputStream mockOutputStream= mock(OutputStream.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenReturn(mockOutputStream);
+
when(mockCompression.getInputStream(any(InputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", STRING_HUGEVALUE,
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING), "{}");
+
+verify(mockCompression,
times(1)).getInputStream(any(InputStream.class));
+
+
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644485936
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -38,24 +45,64 @@
import org.apache.jackrabbit.oak.plugins.memory.StringPropertyState;
import org.apache.jackrabbit.oak.plugins.value.Conversions;
import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* PropertyState implementation with lazy parsing of the JSOP encoded value.
*/
final class DocumentPropertyState implements PropertyState {
+private static final Logger LOG =
LoggerFactory.getLogger(DocumentPropertyState.class);
+
private final DocumentNodeStore store;
private final String name;
private final String value;
private PropertyState parsed;
+private final byte[] compressedValue;
+private final Compression compression;
+
+private static final int DEFAULT_COMPRESSION_THRESHOLD =
SystemPropertySupplier
+.create("oak.documentMK.stringCompressionThreshold ",
-1).loggingTo(LOG).get();
Review Comment:
Done
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
stefan-egli commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644395621
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -128,7 +188,7 @@ public boolean equals(Object object) {
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
return this.name.equals(other.name)
-&& this.value.equals(other.value);
+&& this.getValue().equals(other.getValue());
Review Comment:
Right, I think there should be distinction between non-compressed,
compressed, mixed cases. Only the compressed one is slightly more involved. For
that one we could use a combination of length comparison, hashcode (calculated
when needed) and/or plain Arrays.equals(byte[],byte[])..
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
reschke commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644353388
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -128,7 +188,7 @@ public boolean equals(Object object) {
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
return this.name.equals(other.name)
-&& this.value.equals(other.value);
+&& this.getValue().equals(other.getValue());
Review Comment:
We could special-case the compressed case, and compare the byte arrays.
Still expensive.
Maybe we could keep the String's hashCode in order to avoid comparing the
byte arrays in most cases.
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644348052
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -116,7 +163,20 @@ public int count() {
*/
@NotNull
String getValue() {
-return value;
+return value != null ? value : decompress(this.compressedValue);
+}
+
+private String decompress(byte[] value) {
+try {
+return new String(compression.getInputStream(new
ByteArrayInputStream(value)).readAllBytes(), StandardCharsets.UTF_8);
+} catch (IOException e) {
+LOG.error("Failed to decompress property {} value: ", getName(),
e);
+return "\"{}\"";
+}
+}
+
+private byte[] getCompressedValue() {
Review Comment:
Yes. I did the same idea as Stefan. But Julian insisted to make it
private(it was package-protected). Also, this approach complicated many
tests(trying to solve with reflection)
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
stefan-egli commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644336103
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -116,7 +163,20 @@ public int count() {
*/
@NotNull
String getValue() {
-return value;
+return value != null ? value : decompress(this.compressedValue);
+}
+
+private String decompress(byte[] value) {
+try {
+return new String(compression.getInputStream(new
ByteArrayInputStream(value)).readAllBytes(), StandardCharsets.UTF_8);
+} catch (IOException e) {
+LOG.error("Failed to decompress property {} value: ", getName(),
e);
+return "\"{}\"";
+}
+}
+
+private byte[] getCompressedValue() {
Review Comment:
ah, now I remember. Your comment wasn't visible in this particular review
anymore, so I didn't connect the dots.
I'm actually a fan of facilitating testing, and that can mean to introduce
methods in productive classes that are there solely for testing purpose. That
method can still be non-public but package-protected for example. That in my
view makes things more explicit than using reflection in tests which is
brittle. Moving this method to the test class I assume implies opening up the
actual variable, which seems less favourable.
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
reschke commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644328794
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -116,7 +163,20 @@ public int count() {
*/
@NotNull
String getValue() {
-return value;
+return value != null ? value : decompress(this.compressedValue);
+}
+
+private String decompress(byte[] value) {
+try {
+return new String(compression.getInputStream(new
ByteArrayInputStream(value)).readAllBytes(), StandardCharsets.UTF_8);
+} catch (IOException e) {
+LOG.error("Failed to decompress property {} value: ", getName(),
e);
+return "\"{}\"";
+}
+}
+
+private byte[] getCompressedValue() {
Review Comment:
It actually was my proposal not to expose it. Maybe we can just *copy* or
*move* it to the test class?
--
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-10803 -- compress/uncompress property [jackrabbit-oak]
stefan-egli commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644291222
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -38,24 +45,64 @@
import org.apache.jackrabbit.oak.plugins.memory.StringPropertyState;
import org.apache.jackrabbit.oak.plugins.value.Conversions;
import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* PropertyState implementation with lazy parsing of the JSOP encoded value.
*/
final class DocumentPropertyState implements PropertyState {
+private static final Logger LOG =
LoggerFactory.getLogger(DocumentPropertyState.class);
+
private final DocumentNodeStore store;
private final String name;
private final String value;
private PropertyState parsed;
+private final byte[] compressedValue;
+private final Compression compression;
+
+private static final int DEFAULT_COMPRESSION_THRESHOLD =
SystemPropertySupplier
+.create("oak.documentMK.stringCompressionThreshold ",
-1).loggingTo(LOG).get();
DocumentPropertyState(DocumentNodeStore store, String name, String value) {
+this(store, name, value, Compression.GZIP);
+}
+
+DocumentPropertyState(DocumentNodeStore store, String name, String value,
Compression compression) {
this.store = store;
this.name = name;
-this.value = value;
+if (compression != null && DEFAULT_COMPRESSION_THRESHOLD == -1) {
Review Comment:
if `compression` is null, then it would go into `else` and run into a NPE
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -81,4 +100,179 @@ public void multiValuedBinarySize() throws Exception {
assertEquals(0, reads.size());
}
-}
+@Test
+public void multiValuedAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+List blobs = newArrayList();
+for (int i = 0; i < 13; i++) {
+blobs.add(builder.createBlob(new RandomStream(BLOB_SIZE, i)));
+}
+builder.child(TEST_NODE).setProperty("p", blobs, Type.BINARIES);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.BINARIES, Objects.requireNonNull(p).getType());
+assertEquals(13, p.count());
+
+reads.clear();
+assertEquals(BLOB_SIZE, p.size(0));
+// must not read the blob via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringBelowThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", "dummy", Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(5, p.size(0));
+// must not read the string via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", STRING_HUGEVALUE,
Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(10050, p.size(0));
+// must not read the string via streams
+assertEquals(0, reads.size());
+}
+
+@Test
+public void compressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", "\"" + STRING_HUGEVALUE + "\"",
mockCompression);
+
+
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1644246364 ## RELEASE-NOTES.txt: ## @@ -1,4 +1,4 @@ -Release Notes -- Apache Jackrabbit Oak -- Version 1.64.0 +Release Notes -- Apache Jackrabbit Oak -- Version 1.62.0 Review Comment: Fixed it, thank you for the observation. -- 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
[PR] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
t-rana opened a new pull request, #1542: URL: https://github.com/apache/jackrabbit-oak/pull/1542 …mons -- 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-10898 - Allow AzureCheck and AzureCompact to be built directly wi… [jackrabbit-oak]
dulceanu merged PR #1540: URL: https://github.com/apache/jackrabbit-oak/pull/1540 -- 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] Issue/oak 8848 [jackrabbit-oak]
stefan-egli commented on code in PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1644227441 ## RELEASE-NOTES.txt: ## @@ -1,4 +1,4 @@ -Release Notes -- Apache Jackrabbit Oak -- Version 1.64.0 +Release Notes -- Apache Jackrabbit Oak -- Version 1.62.0 Review Comment: this file shouldn't be part of the PR -- 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-10867: add service principal support in oak-run commons [jackrabbit-oak]
t-rana commented on PR #1513: URL: https://github.com/apache/jackrabbit-oak/pull/1513#issuecomment-2175731261 Closing this PR for now, will raise a new one. -- 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-10867: add service principal support in oak-run commons [jackrabbit-oak]
t-rana closed pull request #1513: OAK-10867: add service principal support in oak-run commons URL: https://github.com/apache/jackrabbit-oak/pull/1513 -- 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-10890 - Added log warning [jackrabbit-oak]
stefan-egli merged PR #1535: URL: https://github.com/apache/jackrabbit-oak/pull/1535 -- 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
[PR] OAK-10901 - Bypass the DocumentNodeState cache when resolving paths downloaded from Mongo. [jackrabbit-oak]
nfsantos opened a new pull request, #1541: URL: https://github.com/apache/jackrabbit-oak/pull/1541 (no comment) -- 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-10894 - DocumentNodeStore: expose readNode as package private [jackrabbit-oak]
nfsantos merged PR #1533: URL: https://github.com/apache/jackrabbit-oak/pull/1533 -- 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] Issues/oak 10781 [jackrabbit-oak]
amit-jain merged PR #1518: URL: https://github.com/apache/jackrabbit-oak/pull/1518 -- 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
[PR] OAK-10898 - Allow AzureCheck and AzureCompact to be built directly wi… [jackrabbit-oak]
dulceanu opened a new pull request, #1540: URL: https://github.com/apache/jackrabbit-oak/pull/1540 …th a CloudBlobDirectory -- 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] Issues/oak 10781 [jackrabbit-oak]
nit0906 commented on code in PR #1518:
URL: https://github.com/apache/jackrabbit-oak/pull/1518#discussion_r1644093746
##
oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureBlobContainerProvider.java:
##
@@ -205,19 +212,33 @@ private CloudBlobContainer
getBlobContainerFromServicePrincipals(@Nullable BlobR
@NotNull
private StorageCredentialsToken getStorageCredentials() {
-ClientSecretCredential clientSecretCredential = new
ClientSecretCredentialBuilder()
-.clientId(clientId)
-.clientSecret(clientSecret)
-.tenantId(tenantId)
-.build();
-AccessToken accessToken = clientSecretCredential.getTokenSync(new
TokenRequestContext().addScopes(AZURE_DEFAULT_SCOPE));
-if (accessToken == null ||
StringUtils.isBlank(accessToken.getToken())) {
-log.error("Access token is null or empty");
-throw new IllegalArgumentException("Could not connect to azure
storage, access token is null or empty");
+boolean isAccessTokenGenerated = false;
+/* generate access token, the same token will be used for subsequent
access
+ * generated token is valid for 1 hour only and will be refreshed in
background
+ * */
+if (accessToken == null) {
+clientSecretCredential = new ClientSecretCredentialBuilder()
+.clientId(clientId)
+.clientSecret(clientSecret)
+.tenantId(tenantId)
+.build();
+accessToken = clientSecretCredential.getTokenSync(new
TokenRequestContext().addScopes(AZURE_DEFAULT_SCOPE));
+if (accessToken == null ||
StringUtils.isBlank(accessToken.getToken())) {
+log.error("Access token is null or empty");
+throw new IllegalArgumentException("Could not connect to azure
storage, access token is null or empty");
+}
+storageCredentialsToken = new StorageCredentialsToken(accountName,
accessToken.getToken());
+isAccessTokenGenerated = true;
+}
+
+Objects.requireNonNull(storageCredentialsToken, "storage credentials
token cannot be null");
+
+// start refresh token executor only when the access token is first
generated
+if (isAccessTokenGenerated) {
+log.info("starting refresh token task at: {}",
OffsetDateTime.now());
+TokenRefresher tokenRefresher = new TokenRefresher();
+executorService.scheduleWithFixedDelay(tokenRefresher,
TOKEN_REFRESHER_INITIAL_DELAY, TOKEN_REFRESHER_DELAY, TimeUnit.MINUTES);
Review Comment:
Can you add a comment here describing why the 1 minute delay is needed.
--
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-10889 - Indexing job: do not remove field with size estimation from NodeDocuments downloaded from Mongo [jackrabbit-oak]
nfsantos merged PR #1530: URL: https://github.com/apache/jackrabbit-oak/pull/1530 -- 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-10874 | Add jmx function for bringing forward a delayed async lane to a latest checkpoint. [jackrabbit-oak]
nit0906 merged PR #1522: URL: https://github.com/apache/jackrabbit-oak/pull/1522 -- 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-10874 | Add jmx function for bringing forward a delayed async lane to a latest checkpoint. [jackrabbit-oak]
nit0906 commented on code in PR #1522:
URL: https://github.com/apache/jackrabbit-oak/pull/1522#discussion_r1643657775
##
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/AsyncIndexUpdate.java:
##
@@ -1242,6 +1242,62 @@ public String getReferenceCheckpoint() {
return referenceCp;
}
+@Override
+public String forceIndexLaneCatchup(String confirmMessage) throws
CommitFailedException {
+
+if (!"CONFIRM".equals(confirmMessage)) {
+String msg = "Please confirm that you want to force the lane
catch-up by passing 'CONFIRM' as argument";
+log.warn(msg);
+return msg;
+}
+
+if (!this.isFailing()) {
+String msg = "The lane is not failing. This operation should
only be performed if the lane is failing, it should first be allowed to catch
up on its own.";
+log.warn(msg);
+return msg;
+}
+
+try {
+log.info("Running a forced catch-up for indexing lane [{}]. ",
name);
+// First we need to abort and pause the running indexing task
+this.abortAndPause();
+log.info("Aborted and paused async indexing for lane [{}]",
name);
+// Release lease for the paused lane
+this.releaseLeaseForPausedLane();
+log.info("Released lease for paused lane [{}]", name);
+String newReferenceCheckpoint = store.checkpoint(lifetime,
Map.of(
Review Comment:
done.
--
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
[PR] JCRVLT-757 Introduce filtering extension SPI [jackrabbit-filevault-package-maven-plugin]
kwin opened a new pull request, #105: URL: https://github.com/apache/jackrabbit-filevault-package-maven-plugin/pull/105 Add filtering feature for escaping FileVault DocView XML Attribute values -- 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: dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1642962427
##
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/VersionEditor.java:
##
@@ -145,27 +146,71 @@ public void propertyChanged(PropertyState before,
PropertyState after)
return;
}
String propName = after.getName();
+
+// Updates the checked-out / checked-in state of the currently
processed node when
+// the JCR_ISCHECKEDOUT property change is processed.
if (propName.equals(JCR_ISCHECKEDOUT)) {
if (wasCheckedIn()) {
vMgr.checkout(node);
} else {
vMgr.checkin(node);
}
} else if (propName.equals(JCR_BASEVERSION)) {
-String baseVersion = after.getValue(Type.REFERENCE);
-if (baseVersion.startsWith(RESTORE_PREFIX)) {
-baseVersion = baseVersion.substring(RESTORE_PREFIX.length());
-node.setProperty(JCR_BASEVERSION, baseVersion, Type.REFERENCE);
+
+// Completes the restore of a version from version history.
+//
+// When the JCR_BASEVERSION property is processed, a check is made
for the current
+// base version property.
+// If a restore is currently in progress for the current base
version (the check for
+// this is that the current base version name has the format
"restore-[UUID of the
+// version to restore to]"), then the restore is completed for the
current node
+// to the version specified by the UUID.
+//
+// If a node that was moved or copied to the location of a deleted
node is currently
+// being processed (see OAK-8848 for context), the restore
operation must NOT be
+// performed when the JCR_BASEVERSION property change is processed
for the node.
+if (!nodeWasMovedOrCopied()) {
+
+String baseVersion = after.getValue(Type.REFERENCE);
+if (baseVersion.startsWith(RESTORE_PREFIX)) {
+baseVersion =
baseVersion.substring(RESTORE_PREFIX.length());
+node.setProperty(JCR_BASEVERSION, baseVersion,
Type.REFERENCE);
+}
+
+vMgr.restore(node, baseVersion, null);
}
-vMgr.restore(node, baseVersion, null);
} else if (isVersionProperty(after)) {
-throwProtected(after.getName());
+// Checks if a version property is being changed and throws a
CommitFailedException
+// with the message "Constraint Violation Exception" if this is
not allowed.
+// JCR_ISCHECKEDOUT and JCR_BASEVERSION properties should be
ignored, since changes
+// to them are allowed for specific use cases (for example,
completing the check-in
+// / check-out for a node or completing a node restore).
+//
+// The only situation when the update of a version property is
allowed is when this
+// occurs as a result of the current node being moved over a
previously deleted node
+// - see OAK-8848 for context.
+//
+// OAK-8848: moving a versionable node in the same location as a
node deleted in the
+// same session should be allowed.
+// This check works because the only way that moving a node in a
location is allowed
+// is if there is no existing (undeleted) node in that location.
+// Property comparison should not fail for two jcr:versionHistory
properties in this case.
+if (!nodeWasMovedOrCopied()) {
+throwProtected(after.getName());
+}
} else if (isReadOnly && getOPV(after) !=
OnParentVersionAction.IGNORE) {
throwCheckedIn("Cannot change property " + after.getName()
+ " on checked in node");
}
}
+/**
+ * Returns true if and only if the given node was moved or copied from
another location.
+ */
+private boolean nodeWasMovedOrCopied() {
Review Comment:
I just tested the copy instead of move case and it can not be done in one
session, because WorkspaceDelegate.copy throws a javax.jcr.ItemExistsException.
Copy was not mentioned in the ticket requirement either, it was a wrong
assumption on my part that copy will behave the same as move in this case.
I will rename the check method to nodeWasMoved.
--
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-10691: remove use of Guava Charsets class [jackrabbit-oak]
reschke merged PR #1538: URL: https://github.com/apache/jackrabbit-oak/pull/1538 -- 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-10897 - Delete unused class: DocumentStoreSplitter [jackrabbit-oak]
nfsantos merged PR #1537: URL: https://github.com/apache/jackrabbit-oak/pull/1537 -- 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-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1642878592
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -81,4 +100,179 @@ public void multiValuedBinarySize() throws Exception {
assertEquals(0, reads.size());
}
-}
+@Test
+public void multiValuedAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+List blobs = newArrayList();
+for (int i = 0; i < 13; i++) {
+blobs.add(builder.createBlob(new RandomStream(BLOB_SIZE, i)));
+}
+builder.child(TEST_NODE).setProperty("p", blobs, Type.BINARIES);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.BINARIES, Objects.requireNonNull(p).getType());
+assertEquals(13, p.count());
+
+reads.clear();
+assertEquals(BLOB_SIZE, p.size(0));
+// must not read the blob via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringBelowThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", "dummy", Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(5, p.size(0));
+// must not read the string via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", STRING_HUGEVALUE,
Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(10050, p.size(0));
+// must not read the string via streams
+assertEquals(0, reads.size());
+}
+
+@Test
+public void compressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", "\"" + STRING_HUGEVALUE + "\"",
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING),
STRING_HUGEVALUE);
+
+verify(mockCompression,
times(1)).getOutputStream(any(OutputStream.class));
+
+compressionThreshold.set(null, -1);
+
+}
+
+@Test
+public void uncompressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+OutputStream mockOutputStream= mock(OutputStream.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenReturn(mockOutputStream);
+
when(mockCompression.getInputStream(any(InputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", STRING_HUGEVALUE,
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING), "{}");
+
+verify(mockCompression,
times(1)).getInputStream(any(InputStream.class));
+
+
[PR] OAK-10896: - Add feature toggle for removal of deleted properties and orphaned nodes [jackrabbit-oak]
shodaaan opened a new pull request, #1539: URL: https://github.com/apache/jackrabbit-oak/pull/1539 - create class FullGCOptions which will hold the different parameters used for running full GC - added variables to Configuration.java for 2 full GC modes: GAP_ORPHANS and EMPTY_PROPERTIES - updated VersionGarbageCollector constructor, as well as DocumentNodeStore Builder / Service and DocumentNodeStore to get values received from Configuration via the FullGCOptions class - applied the new full GC mode options in VersionGarbageCollector via the fullGCOptions parameter passed in the constructor - added unit test methods for testing feature toggle and configuration options set in DocumentNodeStoreBuilder, based on existing testing of feature toggles and configuration settings in unit tests - fixed CommitBuilderTest test failures by changing expected exception type -- 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
