You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by GitBox <gi...@apache.org> on 2022/08/02 12:40:59 UTC
[GitHub] [jackrabbit-oak] mreutegg opened a new pull request, #646: OAK-9850: ConcurrentPrefetchAndUpdateIT.cacheConsistency fails occasi…
mreutegg opened a new pull request, #646:
URL: https://github.com/apache/jackrabbit-oak/pull/646
…onally
Keys collection passed to registerTracker must not be modified later.
NodeDocumentCache.putNonConflictingDocs() must always notify other trackers.
Enable test again.
--
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
[GitHub] [jackrabbit-oak] mreutegg commented on pull request #646: OAK-9850: ConcurrentPrefetchAndUpdateIT.cacheConsistency fails occasi…
Posted by GitBox <gi...@apache.org>.
mreutegg commented on PR #646:
URL: https://github.com/apache/jackrabbit-oak/pull/646#issuecomment-1205152398
Test failure in oak-blob-plugins seems unrelated. I can reproduce the failure locally and created https://issues.apache.org/jira/browse/OAK-9879
--
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
[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #646: OAK-9850: ConcurrentPrefetchAndUpdateIT.cacheConsistency fails occasi…
Posted by GitBox <gi...@apache.org>.
thomasmueller commented on code in PR #646:
URL: https://github.com/apache/jackrabbit-oak/pull/646#discussion_r937451455
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1411,7 +1411,7 @@ public <T extends Document> void prefetch(Collection<T> collection,
List<String> resultKeys = new ArrayList<>(keys.size());
CacheChangesTracker tracker = null;
if (collection == Collection.NODES) {
- tracker = nodesCache.registerTracker(keys);
+ tracker = nodesCache.registerTracker(new HashSet<>(keys));
Review Comment:
Ah, is it because "Keys collection passed to registerTracker must not be modified later" and you wanted to ensure the set is not modified later? I think it's fine... It might make sense to add a comment in the code
--
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
[GitHub] [jackrabbit-oak] stefan-egli commented on pull request #646: OAK-9850: ConcurrentPrefetchAndUpdateIT.cacheConsistency fails occasi…
Posted by GitBox <gi...@apache.org>.
stefan-egli commented on PR #646:
URL: https://github.com/apache/jackrabbit-oak/pull/646#issuecomment-1202971935
Wsa wondering if it would make sense to try and come up with a test case that would have failed without this fix. Maybe it could help finding or explaining other issues (like some stray OakMerge0004 conflicts..).
--
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
[GitHub] [jackrabbit-oak] mreutegg commented on a diff in pull request #646: OAK-9850: ConcurrentPrefetchAndUpdateIT.cacheConsistency fails occasi…
Posted by GitBox <gi...@apache.org>.
mreutegg commented on code in PR #646:
URL: https://github.com/apache/jackrabbit-oak/pull/646#discussion_r937565923
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1411,7 +1411,7 @@ public <T extends Document> void prefetch(Collection<T> collection,
List<String> resultKeys = new ArrayList<>(keys.size());
CacheChangesTracker tracker = null;
if (collection == Collection.NODES) {
- tracker = nodesCache.registerTracker(keys);
+ tracker = nodesCache.registerTracker(new HashSet<>(keys));
Review Comment:
Yes, that's exactly the reason. Good point about the comment. I'll add 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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] mreutegg commented on pull request #646: OAK-9850: ConcurrentPrefetchAndUpdateIT.cacheConsistency fails occasi…
Posted by GitBox <gi...@apache.org>.
mreutegg commented on PR #646:
URL: https://github.com/apache/jackrabbit-oak/pull/646#issuecomment-1204864960
> Was wondering if it would make sense to try and come up with a test case that would have failed without this fix.
Running `ConcurrentPrefetchAndUpdateIT` in a loop eventually failed on my machine without the fix. But I agree, it would be better if there was a deterministic test that fails immediately without the fix.
--
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
[GitHub] [jackrabbit-oak] mreutegg commented on pull request #646: OAK-9850: ConcurrentPrefetchAndUpdateIT.cacheConsistency fails occasi…
Posted by GitBox <gi...@apache.org>.
mreutegg commented on PR #646:
URL: https://github.com/apache/jackrabbit-oak/pull/646#issuecomment-1202893432
I think that's possible. The added else clause in NodeDocumentCache.putNonConflictingDocs() is not specific to prefetching.
--
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
[GitHub] [jackrabbit-oak] mreutegg merged pull request #646: OAK-9850: ConcurrentPrefetchAndUpdateIT.cacheConsistency fails occasi…
Posted by GitBox <gi...@apache.org>.
mreutegg merged PR #646:
URL: https://github.com/apache/jackrabbit-oak/pull/646
--
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
[GitHub] [jackrabbit-oak] mreutegg commented on a diff in pull request #646: OAK-9850: ConcurrentPrefetchAndUpdateIT.cacheConsistency fails occasi…
Posted by GitBox <gi...@apache.org>.
mreutegg commented on code in PR #646:
URL: https://github.com/apache/jackrabbit-oak/pull/646#discussion_r937430348
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1411,7 +1411,7 @@ public <T extends Document> void prefetch(Collection<T> collection,
List<String> resultKeys = new ArrayList<>(keys.size());
CacheChangesTracker tracker = null;
if (collection == Collection.NODES) {
- tracker = nodesCache.registerTracker(keys);
+ tracker = nodesCache.registerTracker(new HashSet<>(keys));
Review Comment:
> Is this to remove duplicates?
No, it just registers the given keys with a `CacheChangesTracker`. Order is not important here. The `CacheChangesTracker` just needs an efficient lookup whether a key is in this set.
> Possibly it makes sense to sort the entries so that MongoDB can retrieve them faster
I don't know if that helps and I would only sort if there is evidence that it is faster.
> I think LZ4 is used?
It _can_ be used. Compression over the wire is a connection [option](https://www.mongodb.com/docs/drivers/java/sync/current/fundamentals/connection/network-compression/#specify-compression-algorithms).
--
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
[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #646: OAK-9850: ConcurrentPrefetchAndUpdateIT.cacheConsistency fails occasi…
Posted by GitBox <gi...@apache.org>.
thomasmueller commented on code in PR #646:
URL: https://github.com/apache/jackrabbit-oak/pull/646#discussion_r937444976
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1411,7 +1411,7 @@ public <T extends Document> void prefetch(Collection<T> collection,
List<String> resultKeys = new ArrayList<>(keys.size());
CacheChangesTracker tracker = null;
if (collection == Collection.NODES) {
- tracker = nodesCache.registerTracker(keys);
+ tracker = nodesCache.registerTracker(new HashSet<>(keys));
Review Comment:
> it just registers the given keys
Yes, but we had "registerTracker(keys)" before and you changed it to "registerTracker(new HashSet<>(keys))" and I wanted to understand the reason for this change
```
- tracker = nodesCache.registerTracker(keys);
+ tracker = nodesCache.registerTracker(new HashSet<>(keys));
```
> I would only sort if there is evidence that it is faster.
Sure.
> Compression over the wire is a connection option
Thanks! So no LZ4, but there's Snappy. (which is a bit slower than LZ4 I think)
--
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
[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #646: OAK-9850: ConcurrentPrefetchAndUpdateIT.cacheConsistency fails occasi…
Posted by GitBox <gi...@apache.org>.
thomasmueller commented on code in PR #646:
URL: https://github.com/apache/jackrabbit-oak/pull/646#discussion_r936801356
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1411,7 +1411,7 @@ public <T extends Document> void prefetch(Collection<T> collection,
List<String> resultKeys = new ArrayList<>(keys.size());
CacheChangesTracker tracker = null;
if (collection == Collection.NODES) {
- tracker = nodesCache.registerTracker(keys);
+ tracker = nodesCache.registerTracker(new HashSet<>(keys));
Review Comment:
Is this to remove duplicates? Using HashSet will re-order the entries, but I guess that's fine.
Possibly it makes sense to sort the entries so that MongoDB can retrieve them faster (a MongoDB specific optimization); I could imagine that MongoDB doesn't do that itself. Or so that compression over the wire can work better (I _think_ LZ4 is used?)
--
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
[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #646: OAK-9850: ConcurrentPrefetchAndUpdateIT.cacheConsistency fails occasi…
Posted by GitBox <gi...@apache.org>.
thomasmueller commented on code in PR #646:
URL: https://github.com/apache/jackrabbit-oak/pull/646#discussion_r937448763
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1411,7 +1411,7 @@ public <T extends Document> void prefetch(Collection<T> collection,
List<String> resultKeys = new ArrayList<>(keys.size());
CacheChangesTracker tracker = null;
if (collection == Collection.NODES) {
- tracker = nodesCache.registerTracker(keys);
+ tracker = nodesCache.registerTracker(new HashSet<>(keys));
Review Comment:
Please ignore my comments about compression and ordering... I see now this doesn't apply here.
But "keys" is already "Set<String> keys = new HashSet<>();", and you wrapped that inside a new HashSet, what is the reason?
--
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
[GitHub] [jackrabbit-oak] mreutegg commented on pull request #646: OAK-9850: ConcurrentPrefetchAndUpdateIT.cacheConsistency fails occasi…
Posted by GitBox <gi...@apache.org>.
mreutegg commented on PR #646:
URL: https://github.com/apache/jackrabbit-oak/pull/646#issuecomment-1206548350
> a test case that would have failed without this fix
@stefan-egli there is now a simple test that illustrates why changes to NodeDocumentCache are necessary. `NodeDocumentCacheTest` fails deterministically without changes to `NodeDocumentCache.putNonConflictingDocs()`.
--
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