MNT-18308 force async acl creation #987

evasques · 2020-05-11T09:37:07Z

Replaces PR #984

Implements solution provided by @killerboot :

When time is exceeded (set on fixedAclMaxTransactionTime), it shall always turn the call async and delegate ACL creation to the job
Fix the current async behavior to also delegate the current's branch children to the job instead of processing them because they have no children

Aditionally also contains:

Fix to the FixedACLUpdater job: it was picking up nodes in batches and kept resetting a maxNodeId with the last node of the list so on next iteration it would not pick those up again. Problem is the list is sorted desc, so smallest ID comes last - so the next iteration was picking up everything again except the lowest nodeID from the previous. This caused no error on the job, but threw null pointers as it tried to re-process nodes.
Unit test to assert that when time is exceeded in a sync call, the call with turn async, there will be nodes with aspect ASPECT_PENDING_FIX_ACL to be processed by job and job will then process them correctly.

…st was sorted desc, Fixed to reset maxNode only when nodeId is greater than last

killerboot · 2020-05-11T10:20:33Z

src/main/java/org/alfresco/repo/domain/permissions/FixedAclUpdater.java

@@ -321,7 +321,9 @@ public boolean handle(Pair<Long, NodeRef> nodePair)
            if (nodes.size() < maxItemBatchSize)
            {
                nodes.add(nodePair.getSecond());
-                maxNodeId = nodePair.getFirst();
+                if(nodePair.getFirst() > maxNodeId) {


Please check the formatting settings of your IDE. The braces should be on new lines and a whitespace between "if" and the condition clause.

killerboot · 2020-05-11T10:25:04Z

src/main/java/org/alfresco/repo/domain/permissions/ADMAccessControlListDAO.java

+        AlfrescoTransactionSupport.bindResource(FixedAclUpdater.FIXED_ACL_ASYNC_REQUIRED_KEY, true);
+        // stop propagating on children nodes
+        return false;
+


Please remove unnecessary empty lines.

killerboot · 2020-05-11T10:26:17Z

src/main/java/org/alfresco/repo/domain/permissions/ADMAccessControlListDAO.java

+     * If async call required adds ASPECT_PENDING_FIX_ACL aspect to nodes when transactionTime reaches max admitted time
+     * MNT-18308: No longer checks if call is async in order to evaluate time passed to decide if nodes should be processed by job.
+     * This is now the default behavior for both sync and async calls: when fixedAclMaxTransactionTime is exceeded, call turns
+     * async and all remaining nodes should be processed by job FixedACLUpdater


Can you please add a description of the method signature in this JavaDoc (parameters and return value description)?

killerboot · 2020-05-11T10:29:11Z

src/test/java/org/alfresco/repo/domain/permissions/FixedAclUpdaterTest.java

@@ -68,7 +68,8 @@
    private FileFolderService fileFolderService;
    private Repository repository;
    private FixedAclUpdater fixedAclUpdater;
-    private NodeRef folderNodeRef;
+    private NodeRef mnt15368folder;


Please remove references to JIRA tickets from variables. Use meaningful names instead.

killerboot · 2020-05-11T10:29:26Z

src/test/java/org/alfresco/repo/domain/permissions/FixedAclUpdaterTest.java

@@ -225,6 +231,66 @@ public Void execute() throws Throwable
            }
        }, false, true);
    }
+
+    @Test
+    public void testMNT18308()


Please remove references to JIRA tickets from methods. Use meaningful names instead.

Is it possible to unite this test with testMNT15368 to avoid code duplication?

testMNT15368 is setting inherit permissions forcing async: permissionService.setInheritParentPermissions(mnt15368folder, false, true);

In order to test MNT-18308 we need to set them sync:
permissionService.setInheritParentPermissions(mnt18308folder, false, false);

In fact, with these changes now, the behavior is exactly the same, so the async/sync flag does not make a difference as async calls behave the same as sync calls until time is exceeded and now sync calls behave the same as async calls when time was exceeded.

testMNT15368 was kept to assert previous behavior is the same on async calls
testMNT18308 was added to assure that a sync call turns async after transaction time was exceeded.

I was just trying to highlight that the tests are only different in the first part. The majority of the test is a copy-paste which is discouraged.

You are right, but i cannot reproduce both issues by reapplying the permissions to the previous tree the other test ran on. I'm not sure how I could unite them. Best I can do I think create methods that both call. Do you agree or did you have a better solution in mind?

src/main/java/org/alfresco/repo/domain/permissions/ADMAccessControlListDAO.java

killerboot · 2020-05-11T15:36:54Z

src/main/java/org/alfresco/repo/domain/permissions/ADMAccessControlListDAO.java

+        if (log.isWarnEnabled() && (AlfrescoTransactionSupport.getResource(FixedAclUpdater.FIXED_ACL_ASYNC_REQUIRED_KEY) == null
+                || (Boolean) AlfrescoTransactionSupport.getResource(FixedAclUpdater.FIXED_ACL_ASYNC_REQUIRED_KEY) == false))
+        {
+            log.warn("Time was exceeded in transaction " + AlfrescoTransactionSupport.getTransactionId()


I would like to propose to change the message to be a bit more precise:
"The ACL processing time in transaction X exceeded the configured limit of Y ms. The rest of the ACL processing will be done asynchronously."
I guess it is ok to omit some of the information, as the elapsed time (transactionTime) will be very close to the limit and the exact date when this happened will be in the log statement anyways.

killerboot

Overall looks good. Please see my comment about the log message and the test. Other than that it is ok to merge.

evasques added 2 commits May 11, 2020 10:24

Job was picking up all nodes but first node from previous batch as li…

9ad5fb4

…st was sorted desc, Fixed to reset maxNode only when nodeId is greater than last

MNT-18308 ACL creation must turn async if time is exceeded and unit test

cb59965

evasques requested review from jottley and killerboot May 11, 2020 09:37

evasques mentioned this pull request May 11, 2020

MNT-18308 - Add flag forceAsyncAclCreation #984

Open

killerboot suggested changes May 11, 2020

View reviewed changes

evasques added 3 commits May 11, 2020 15:43

Fixed formatting

432a6b9

Changed variable names of folder NodeRefs

5a4f54b

added javadoc and warning when sync call turns async

78162a3

evasques requested a review from killerboot May 11, 2020 14:48

killerboot reviewed May 11, 2020

View reviewed changes

killerboot approved these changes May 11, 2020

View reviewed changes

evasques added 2 commits May 11, 2020 18:33

Refactored the Unit Tests

d509d76

Changed warning message for when ACL processing turns async

127069d

jottley approved these changes May 11, 2020

View reviewed changes

fixed formatting

0976bea

evasques merged commit c1c0650 into master May 14, 2020

pr-triage bot added the PR: merged label May 14, 2020

evasques deleted the fix/MNT-18308_force_async_ACL_creation branch May 21, 2020 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNT-18308 force async acl creation #987

MNT-18308 force async acl creation #987

evasques commented May 11, 2020

killerboot May 11, 2020

killerboot May 11, 2020

killerboot May 11, 2020

killerboot May 11, 2020

killerboot May 11, 2020

killerboot May 11, 2020

evasques May 11, 2020

killerboot May 11, 2020

evasques May 11, 2020

killerboot May 11, 2020

killerboot left a comment

MNT-18308 force async acl creation #987

MNT-18308 force async acl creation #987

Conversation

evasques commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killerboot left a comment

Choose a reason for hiding this comment