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-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-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
[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
