[GitHub] [jackrabbit-oak] mreutegg commented on a diff in pull request #796: OAK-10031 : purge uncommitted revisions and collisions in batches on …

[GitBox <gi...@apache.org>]

mreutegg commented on code in PR #796:
URL: https://github.com/apache/jackrabbit-oak/pull/796#discussion_r1051894822


##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java:
##########
@@ -563,67 +565,81 @@ public boolean containsRevision(@NotNull Revision revision) {
      * {@link Utils#isCommitted(String)} returns false.
      *
      * <p>
-     *     <bold>Note</bold> - This method should only be invoked upon startup
-     *     as then only we can safely assume that these revisions would not be
-     *     committed
+     * <bold>Note</bold> - This method should only be invoked upon startup
+     * as then only we can safely assume that these revisions would not be
+     * committed
      * </p>
      *
-     * @param context the revision context.
+     * @param context                   the revision context.
+     * @param batchSize                 the batch size to purge uncommitted revisions
      * @return count of the revision entries purged
      */
-    int purgeUncommittedRevisions(RevisionContext context) {
+    int purgeUncommittedRevisions(RevisionContext context, int batchSize) {
         // only look at revisions in this document.
         // uncommitted revisions are not split off
         Map<Revision, String> localRevisions = getLocalRevisions();
-        UpdateOp op = new UpdateOp(getId(), false);
+
         Set<Revision> uniqueRevisions = new HashSet<>();
-        for (Map.Entry<Revision, String> commit : localRevisions.entrySet()) {
-            if (!Utils.isCommitted(commit.getValue())) {
-                Revision r = commit.getKey();
-                if (r.getClusterId() == context.getClusterId()) {
-                    uniqueRevisions.add(r);
-                    removeRevision(op, r);
-                }
+
+        for (List<Entry<Revision, String>> localRevisionsList : partition(localRevisions.entrySet(), batchSize)) {

Review Comment:
   This partitions the revisions present on the documents, whereas I think the actual number of removed revisions should be partitioned. Depending on how uncommitted revisions appear in `localRevisions`, the proposed code passes `UpdateOp` to `findAndUpdate()` with potentially fewer changes than `batchSize`.



-- 
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