[GitHub] [jackrabbit-oak] stefan-egli commented on a diff in pull request #753: OAK-9993: Add utility method to remove unmerged branches

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

stefan-egli commented on code in PR #753:
URL: https://github.com/apache/jackrabbit-oak/pull/753#discussion_r1022655229


##########
oak-run/src/main/js/oak-mongo.js:
##########
@@ -452,6 +452,70 @@ var oak = (function(global){
         }
     };
 
+    /**
+     * Removes all unmerged branches on the document with the given path and
+     * clusterId. This method will only remove unmerged branches when the
+     * clusterId is inactive.

Review Comment:
   Should we add a comment a la
   "This corresponds to DocumentNodeStore.cleanOrphanedBranches (which is part of a startup and normal background update)"



##########
oak-run/src/main/js/oak-mongo.js:
##########
@@ -452,6 +452,70 @@ var oak = (function(global){
         }
     };
 
+    /**
+     * Removes all unmerged branches on the document with the given path and
+     * clusterId. This method will only remove unmerged branches when the
+     * clusterId is inactive.
+     *
+     * @memberof oak
+     * @method removeUnmergedBranches
+     * @param {string} path the path of a document
+     * @param {number} clusterId collision markers for this clusterId will be removed.
+     * @param {number} [limit=1000000] maximum number of unmerged branches to remove.
+     * @returns {object} the result of the MongoDB update.
+     */
+    api.removeUnmergedBranches = function(path, clusterId, limit) {
+        if (path === undefined) {
+            print("No path specified");
+            return;
+        }
+        if (clusterId === undefined) {
+            print("No clusterId specified");
+            return;
+        }
+        if (limit === undefined) {
+            limit = 1000000;
+        }
+        // refuse to remove when clusterId is marked active
+        var clusterNode = db.clusterNodes.findOne({_id: clusterId.toString()});
+        if (clusterNode && clusterNode.state == "ACTIVE") {
+            print("Cluster node with id " + clusterId + " is active!");
+            print("Can only remove unmerged branches for inactive cluster node.");
+            return;
+        }

Review Comment:
   What I was always wondering is whether these utility methods that require inactive clusterNodes (this and the removeCollisions one) should actually acquire the lock and only proceed if that was successful.
   * they could set themselves to `invisible` to avoid discovery interference
   * they could set the lease timeout high enough, so that under most circumstances it is able to finish in that time without complicated lease update logic in this method (eg 5min?)
   * they should release the lock upon finishing (successful or when failing)
   * in case they'd fail it would leave the lock for as many minutes we've chosen the lease timeout to - but as it is only an invisible lock, that wouldn't really do much harm (but maybe we'd need a method to force-unlock too?)
   
   The risk we currently have is that there is interference with an automatically triggered startup of a node (perhaps unknowingly/unnoticed) which happens to pick this clusterNodeId .. and depending on logic it could or could not be a problem..
   
   If we'd have the lease-lock in these javascript methods, it might also allow to implement larger cleanups of many collisions/branch-commits (for which we currently go via `limit`)



##########
oak-run/src/main/js/oak-mongo.js:
##########
@@ -401,7 +401,7 @@ var oak = (function(global){
      * @method removeCollisions
      * @param {string} path the path of a document
      * @param {number} clusterId collision markers for this clusterId will be removed.
-     * @param {number} (optional) limit maximum number of collision markers to remove, default is 1000000.
+     * @param {number} [limit=1000000] maximum number of collision markers to remove.
      * @returns {object} the result of the MongoDB update.
      */
     api.removeCollisions = function(path, clusterId, limit) {

Review Comment:
   Similar to the other reference to java code, should we add something like
   "This corresponds to DocumentNodeStore.cleanRootCollisions (which is part of a startup and normal background update)"



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