Joscorbe commented on code in PR #1377:
URL: https://github.com/apache/jackrabbit-oak/pull/1377#discussion_r1540903551
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##########
@@ -1714,7 +1736,48 @@ public void removeGarbage(final VersionGCStats stats) {
}
}
- private boolean verify(NodeDocument oldDoc, NodeDocument newDoc,
UpdateOp update) {
+ private boolean verifyViaTraversedState(NodeState traversedState,
NodeState traversedParent,
+ NodeDocument newDoc) {
+ final Path path = newDoc.getPath();
+ final Revision lastRevision =
nodeStore.getPendingModifications().get(path);
+ if (traversedParent == null && !newDoc.getPath().isRoot()) {
+ log.error("verify : no parent but not root for path : {}",
newDoc.getPath());
+ return false;
+ }
+ final RevisionVector lastRev;
+ if (traversedParent == null && newDoc.getPath().isRoot()) {
+ if (!(traversedState instanceof DocumentNodeState)) {
+ log.error("verify : traversedState not a DocumentNodeState
: {}",
+ traversedState.getClass());
+ return false;
+ }
+ lastRev = ((DocumentNodeState)
traversedState).getLastRevision();
+ } else {
+ if (!traversedParent.exists()) {
+ // if the parent doesn't exist we shouldn't reach this
point at all
+ log.error("verify : no parent but not marked for removal
for path : {}", newDoc.getPath());
+ return false;
+ }
+ if (!(traversedParent instanceof DocumentNodeState)) {
+ log.error("verify : traversedParent not a
DocumentNodeState : {}",
+ traversedParent.getClass());
+ return false;
+ }
+ lastRev = ((DocumentNodeState)
traversedParent).getLastRevision();
+ }
+ final NodeState actual = newDoc.getNodeAtRevision(nodeStore,
lastRev, lastRevision);
+ // use more thorough version of equals to ensure properties are
checked
+ // (the faster state.equals() would stop if lastRev matches,
+ // but as we're fiddling with immuability rule of a document,
Review Comment:
```suggestion
// but as we're fiddling with immutability rule of a document,
```
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##########
@@ -1639,16 +1640,37 @@ public void removeGarbage(final VersionGCStats stats) {
update.getId());
continue;
}
+ NodeState traversedParent = null;
+ NodeState traversedState = root;
+ for (String name : oldDoc.getPath().elements()) {
+ traversedParent = traversedState;
+ traversedState = traversedState.getChildNode(name);
+ }
+
NodeDocument newDoc = Collection.NODES.newDocument(ds);
oldDoc.deepCopy(newDoc);
UpdateUtils.applyChanges(newDoc, update);
- if (!verify(oldDoc, newDoc, update)) {
+ // for the time being, verify both with classic and
traversed
+ if (!verifyViaTraversedState(traversedState,
traversedParent,
+ newDoc)) {
// verification failure
// let's skip this document
it.remove();
stats.skippedDetailedGCDocsCount++;
}
};
+ for (Entry<String, Long> e :
orphanOrDeletedRemovalMap.entrySet()) {
+ NodeState traversedState = root;
+ for (String name :
Path.fromString(Utils.getPathFromId(e.getKey())).elements()) {
+ traversedState = traversedState.getChildNode(name);
+ }
+ if (!verifyDeletion(traversedState)) {
+ // verification failure
+ // let's skip this document
+ it.remove();
Review Comment:
Same here
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##########
@@ -1639,16 +1640,37 @@ public void removeGarbage(final VersionGCStats stats) {
update.getId());
continue;
}
+ NodeState traversedParent = null;
+ NodeState traversedState = root;
+ for (String name : oldDoc.getPath().elements()) {
+ traversedParent = traversedState;
+ traversedState = traversedState.getChildNode(name);
+ }
+
NodeDocument newDoc = Collection.NODES.newDocument(ds);
oldDoc.deepCopy(newDoc);
UpdateUtils.applyChanges(newDoc, update);
- if (!verify(oldDoc, newDoc, update)) {
+ // for the time being, verify both with classic and
traversed
+ if (!verifyViaTraversedState(traversedState,
traversedParent,
Review Comment:
Should we maybe add a debug line here? We may have interest in detecting
these cases. Wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
