stefan-egli commented on code in PR #920:
URL: https://github.com/apache/jackrabbit-oak/pull/920#discussion_r1196705181
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1147,6 +1155,85 @@ private <T extends Document> T
findAndModify(Collection<T> collection,
}
}
+ /**
+ * Performs a conditional update (e.g. using
+ * {@link Condition.Type#EXISTS} and only update the
+ * document if the condition is <code>true</code>. The returned documents
are
+ * immutable.
+ * <p>
+ * In case of a {@code DocumentStoreException} (e.g. when a communication
+ * error occurs) only some updates may have been applied. In this case
+ * it is the responsibility of the caller to check which {@link UpdateOp}s
+ * were applied and take appropriate action. The implementation however
ensures
+ * that the result of the operations are properly reflected in the document
+ * cache. That is, an implementation could simply evict the documents
related
+ * to the given update operations from the cache.
+ *
+ * @param collection the collection
+ * @param updateOps the update operation List
+ * @return the list containing old documents or <code>null</code> if the
condition is not met
+ * or if the document wasn't found. The order in the result list reflects
the order
+ * in the updateOps parameter
+ * @throws DocumentStoreException if the operation failed. E.g. because of
+ * an I/O error.
+ * @see #createOrUpdate(Collection, List)
+ */
+ @Override
+ @NotNull
+ public <T extends Document> List<T> findAndUpdate(final @NotNull
Collection<T> collection, final @NotNull List<UpdateOp> updateOps) {
Review Comment:
I agree those boolean flags might not be most user friendly. However, the
MongoDocumentStore already works that way, i.e. people reading this class will
already be somewhat familiar with them - in particular if they are the same for
`<single>` and `<bulk>`.
Some additional aspects relevant for this discussion:
* `sendBulkModify` and `sendBulkUpdate` are actually quite similar. I
haven't tried it, but it seems like it could be deduplicated with perhaps two
boolean flags. If that were the case, i think the gain would be quite large -
as we'd only have to maintain 1 variant.
* The other pairs indeed differ a bit more - but I would still see an
advantage with deduplication based solely on maintenance (unless the code then
becomes entirely unreadable)
* One more related point I think is test coverage. `createOrUpdate<bulk>`
has a fair amount of test coverage which includes dedicated classes
`BulkCreateOrUpdateTest` and `BulkCreateOrUpdateClusterTest` (besides obviously
being used in several higher level use cases). Given the new
`findAndUpdate<bulk>` would be new, it would be actually unused at this stage -
so it might not immediately require the same test coverage. Having said that, I
would argue we should at least look into adding some more test coverage
(perhaps using the `createOrUpdate<bulk>` variants as input for ideas or even
generalize them (but I think the latter is not necessarily possible).
* If we had full deduplication however, then we would profit from the test
coverage for `createOrUpdate<bulk>` to some extend. That could be useful to
have as a starting point.
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
