You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "stefan-egli (via GitHub)" <gi...@apache.org> on 2024/03/25 17:35:20 UTC
[PR] OAK-10715 : embedded verification fixes : it was using the wrong equa… [jackrabbit-oak]
stefan-egli opened a new pull request, #1377:
URL: https://github.com/apache/jackrabbit-oak/pull/1377
…ls method that always returned true in the given use case, plus added verifyDeletion
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10715 : embedded verification fixes : it was using the wrong equa… [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1377:
URL: https://github.com/apache/jackrabbit-oak/pull/1377#discussion_r1540935354
##########
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:
+1, added https://github.com/apache/jackrabbit-oak/pull/1377/commits/5b668577a5712f48fa569d998e99ae3fc302c362
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10715 : embedded verification fixes : it was using the wrong equa… [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10715 : embedded verification fixes : it was using the wrong equa… [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli merged PR #1377:
URL: https://github.com/apache/jackrabbit-oak/pull/1377
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10715 : embedded verification fixes : it was using the wrong equa… [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1377:
URL: https://github.com/apache/jackrabbit-oak/pull/1377#discussion_r1540935123
##########
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:
+1, added https://github.com/apache/jackrabbit-oak/pull/1377/commits/5b668577a5712f48fa569d998e99ae3fc302c362
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org