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
phases.stop(GCPhase.FULL_GC_COLLECT_ORPHAN_NODES);
return false;
}
+
+ // first check if gapOrphansModeEnabled is set in fullGCOptions,
then check in fullGCMode enum
Review Comment:
but the comment is still 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
