mreutegg commented on a change in pull request #258:
URL: https://github.com/apache/jackrabbit-oak/pull/258#discussion_r520594302
##########
File path:
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/MissingLastRevSeekerTest.java
##########
@@ -216,4 +220,26 @@ public void getClusterNodeInfo() {
private ClusterNodeInfoDocument getClusterNodeInfo(int clusterId) {
return seeker.getClusterNodeInfo(clusterId);
}
+
+ @Test
+ public void getNonSplitDocs() throws Exception {
+ String nodeName = this.getClass().getName() + "-foo";
+ DocumentNodeStore dns =
getBuilder().setDocumentStore(store).getNodeStore();
+ NodeBuilder b1 = dns.getRoot().builder();
+ b1.child(nodeName);
+ dns.merge(b1, EmptyHook.INSTANCE, CommitInfo.EMPTY);
+ //Modify and commit changes on this node 100 times to create a split
document
+ for (int i = 0; i < NUM_REVS_THRESHOLD; i++) {
+ b1 = dns.getRoot().builder();
+ b1.child(nodeName).setProperty("prop",i);
+ dns.merge(b1, EmptyHook.INSTANCE, CommitInfo.EMPTY);
+ }
+ dns.runBackgroundOperations();
+ int docs = 0;
+ //seeker should return only non split documents
+ for(Document doc : seeker.getCandidates(0)) {
+ docs++;
+ }
+ assertEquals(2, docs);
+ }
Review comment:
The DocumentNodeStore is not disposed.
```suggestion
dns.dispose();
}
```
##########
File path:
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/BasicDocumentStoreTest.java
##########
@@ -54,6 +55,18 @@ public BasicDocumentStoreTest(DocumentStoreFixture dsf) {
super(dsf);
}
+ @After
+ public void tearDown() {
+ Revision.resetClockToDefault();
+ markDocumentsForCleanup();
+ }
+
+ private void markDocumentsForCleanup() {
+ for (NodeDocument doc : Utils.getAllDocuments(ds)) {
+ removeMe.add(doc.getId());
+ }
+ }
+
Review comment:
Why is this needed in BasicDocumentStoreTest? AFAICS none of the tests
uses a Clock. I think this should rather go to `MissingLastRevSeekerTest` and
clean up documents created by the new `getNonSplitDocs()` test.
##########
File path:
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/MissingLastRevSeekerTest.java
##########
@@ -216,4 +220,26 @@ public void getClusterNodeInfo() {
private ClusterNodeInfoDocument getClusterNodeInfo(int clusterId) {
return seeker.getClusterNodeInfo(clusterId);
}
+
+ @Test
+ public void getNonSplitDocs() throws Exception {
+ String nodeName = this.getClass().getName() + "-foo";
+ DocumentNodeStore dns =
getBuilder().setDocumentStore(store).getNodeStore();
+ NodeBuilder b1 = dns.getRoot().builder();
+ b1.child(nodeName);
+ dns.merge(b1, EmptyHook.INSTANCE, CommitInfo.EMPTY);
+ //Modify and commit changes on this node 100 times to create a split
document
+ for (int i = 0; i < NUM_REVS_THRESHOLD; i++) {
+ b1 = dns.getRoot().builder();
+ b1.child(nodeName).setProperty("prop",i);
+ dns.merge(b1, EmptyHook.INSTANCE, CommitInfo.EMPTY);
+ }
+ dns.runBackgroundOperations();
Review comment:
When triggering background operations manually, the background threads
should be disabled on the DocumentNodeStore with
`DocumentNodeStoreBuilder.setAsyncDelay(0)`.
##########
File path:
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/MissingLastRevSeekerTest.java
##########
@@ -216,4 +220,26 @@ public void getClusterNodeInfo() {
private ClusterNodeInfoDocument getClusterNodeInfo(int clusterId) {
return seeker.getClusterNodeInfo(clusterId);
}
+
+ @Test
+ public void getNonSplitDocs() throws Exception {
+ String nodeName = this.getClass().getName() + "-foo";
+ DocumentNodeStore dns =
getBuilder().setDocumentStore(store).getNodeStore();
+ NodeBuilder b1 = dns.getRoot().builder();
+ b1.child(nodeName);
+ dns.merge(b1, EmptyHook.INSTANCE, CommitInfo.EMPTY);
+ //Modify and commit changes on this node 100 times to create a split
document
+ for (int i = 0; i < NUM_REVS_THRESHOLD; i++) {
+ b1 = dns.getRoot().builder();
+ b1.child(nodeName).setProperty("prop",i);
+ dns.merge(b1, EmptyHook.INSTANCE, CommitInfo.EMPTY);
+ }
+ dns.runBackgroundOperations();
Review comment:
At this point the test should remember the documents for the cleanup.
```suggestion
dns.runBackgroundOperations();
// remember all documents for cleanup
for (NodeDocument doc : Utils.getAllDocuments(ds)) {
removeMe.add(doc.getId());
}
```
##########
File path:
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/MissingLastRevSeekerTest.java
##########
@@ -216,4 +220,26 @@ public void getClusterNodeInfo() {
private ClusterNodeInfoDocument getClusterNodeInfo(int clusterId) {
return seeker.getClusterNodeInfo(clusterId);
}
+
+ @Test
+ public void getNonSplitDocs() throws Exception {
+ String nodeName = this.getClass().getName() + "-foo";
+ DocumentNodeStore dns =
getBuilder().setDocumentStore(store).getNodeStore();
Review comment:
The DocumentNodeStore should use the clock setup by this test. Otherwise
it can be unstable because `Revision` and `ClusterNodeInfo` are using the
virtual clock. Unfortunately it is also necessary to wrap the store and prevent
disposal of the underlying store.
```suggestion
DocumentNodeStore dns = getBuilder().clock(clock).setAsyncDelay(0)
.setDocumentStore(new DocumentStoreWrapper(store) {
@Override
public void dispose() {
// do not close underlying store, otherwise cleanup
// cannot remove documents after the test
}
}).getNodeStore();
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
