From 50da5e21566505385d2094a431636fabaf5bbe69 Mon Sep 17 00:00:00 2001 From: Epure Alexandru-Eusebiu Date: Wed, 7 Apr 2021 12:41:56 +0300 Subject: [PATCH 1/2] MNT-22310 : Performance bottleneck in Disposition Lifecycle Fix the single db transaction by requesting a new transaction each time doInTransaction is called Added configurable batch size property DispositionLifecycleJobExecuter#executeAction now uses a List instead of a single NodeRef Fix UnitTests --- .../alfresco-global.properties | 5 ++ .../org_alfresco_module_rm/rm-job-context.xml | 1 + .../job/DispositionLifecycleJobExecuter.java | 81 ++++++++++--------- ...spositionLifecycleJobExecuterUnitTest.java | 65 ++++++++++++--- 4 files changed, 100 insertions(+), 52 deletions(-) diff --git a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/alfresco-global.properties b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/alfresco-global.properties index 0df8fa38c8..5c3ba95bde 100644 --- a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/alfresco-global.properties +++ b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/alfresco-global.properties @@ -52,6 +52,11 @@ rm.autocompletesuggestion.nodeParameterSuggester.aspectsAndTypes=rma:record,cm:c # rm.dispositionlifecycletrigger.cronexpression=0 0/5 * * * ? +# +# Global RM retention lifecycle cron job execution batch size +# +rm.dispositionlifecycletrigger.batchsize=500 + # # Global RM notify of records due for review cron job expression # diff --git a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-job-context.xml b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-job-context.xml index 047aaa4a70..139ffbd577 100644 --- a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-job-context.xml +++ b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-job-context.xml @@ -80,6 +80,7 @@ + diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuter.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuter.java index a19913db4e..b44cf229d3 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuter.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuter.java @@ -60,6 +60,9 @@ public class DispositionLifecycleJobExecuter extends RecordsManagementJobExecute /** logger */ private static Log logger = LogFactory.getLog(DispositionLifecycleJobExecuter.class); + /** batching properties */ + private int batchSize; + /** list of disposition actions to automatically execute */ private List dispositionActions; @@ -88,6 +91,11 @@ public class DispositionLifecycleJobExecuter extends RecordsManagementJobExecute this.dispositionActions = dispositionActions; } + public void setBatchSize(int batchSize) + { + this.batchSize = batchSize; + } + /** * @param recordsManagementActionService records management action service */ @@ -167,13 +175,14 @@ public class DispositionLifecycleJobExecuter extends RecordsManagementJobExecute { boolean hasMore = true; int skipCount = 0; - while(hasMore) + while (hasMore) { SearchParameters params = new SearchParameters(); params.addStore(StoreRef.STORE_REF_WORKSPACE_SPACESSTORE); params.setLanguage(SearchService.LANGUAGE_FTS_ALFRESCO); params.setQuery(getQuery()); params.setSkipCount(skipCount); + params.setMaxItems(batchSize); // execute search ResultSet results = searchService.query(params); @@ -188,13 +197,12 @@ public class DispositionLifecycleJobExecuter extends RecordsManagementJobExecute } // process search results - for (NodeRef node : resultNodes) + if (!resultNodes.isEmpty()) { - executeAction(node); + executeAction(resultNodes); } } } - logger.debug("Job Finished"); } catch (AlfrescoRuntimeException exception) @@ -209,57 +217,52 @@ public class DispositionLifecycleJobExecuter extends RecordsManagementJobExecute /** * Helper method that executes a disposition action * - * @param actionNode - the disposition action to execute + * @param actionNodes - the disposition actions to execute */ - private void executeAction(final NodeRef actionNode) + private void executeAction(final List actionNodes) { - RetryingTransactionCallback processTranCB = new RetryingTransactionCallback() - { - public Boolean execute() + RetryingTransactionCallback processTranCB = () -> { + for (NodeRef actionNode : actionNodes) { - final String dispAction = (String) nodeService.getProperty(actionNode, - RecordsManagementModel.PROP_DISPOSITION_ACTION); - - // Run disposition action - if (dispAction != null && dispositionActions.contains(dispAction)) + if (nodeService.exists(actionNode)) { - ChildAssociationRef parent = nodeService.getPrimaryParent(actionNode); - if (parent.getTypeQName().equals(RecordsManagementModel.ASSOC_NEXT_DISPOSITION_ACTION)) + final String dispAction = (String) nodeService + .getProperty(actionNode, RecordsManagementModel.PROP_DISPOSITION_ACTION); + + // Run disposition action + if (dispAction != null && dispositionActions.contains(dispAction)) { - Map props = new HashMap<>(1); - props.put(RMDispositionActionExecuterAbstractBase.PARAM_NO_ERROR_CHECK, - Boolean.FALSE); - - try + ChildAssociationRef parent = nodeService.getPrimaryParent(actionNode); + if (parent.getTypeQName().equals(RecordsManagementModel.ASSOC_NEXT_DISPOSITION_ACTION)) { - // execute disposition action - recordsManagementActionService.executeRecordsManagementAction( - parent.getParentRef(), dispAction, props); + Map props = new HashMap<>(1); + props.put(RMDispositionActionExecuterAbstractBase.PARAM_NO_ERROR_CHECK, Boolean.FALSE); - if (logger.isDebugEnabled()) + try { - logger.debug("Processed action: " + dispAction + "on" + parent); + // execute disposition action + recordsManagementActionService + .executeRecordsManagementAction(parent.getParentRef(), dispAction, props); + + if (logger.isDebugEnabled()) + { + logger.debug("Processed action: " + dispAction + "on" + parent); + } } - } - catch (AlfrescoRuntimeException exception) - { - if (logger.isDebugEnabled()) + catch (AlfrescoRuntimeException exception) { - logger.debug(exception); + if (logger.isDebugEnabled()) + { + logger.debug(exception); + } } } } } - - return Boolean.TRUE; } + return Boolean.TRUE; }; - - // if exists - if (nodeService.exists(actionNode)) - { - retryingTransactionHelper.doInTransaction(processTranCB); - } + retryingTransactionHelper.doInTransaction(processTranCB, false, true); } public PersonService getPersonService() diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuterUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuterUnitTest.java index 08c40e50b4..30b3f82ee7 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuterUnitTest.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuterUnitTest.java @@ -33,6 +33,7 @@ import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyMap; import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -56,6 +57,7 @@ import org.junit.Before; import org.junit.Test; import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; +import org.mockito.Matchers; import org.mockito.Mock; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; @@ -91,9 +93,24 @@ public class DispositionLifecycleJobExecuterUnitTest extends BaseUnitTest { super.before(); + // Because of the fix implemented in MNT-22310, a new setup for retrying transaction helper is required. + Answer doInTransactionAnswer = new Answer() + { + @SuppressWarnings("rawtypes") + @Override + public Object answer(InvocationOnMock invocation) throws Throwable + { + RetryingTransactionCallback callback = (RetryingTransactionCallback)invocation.getArguments()[0]; + return callback.execute(); + } + }; + doAnswer(doInTransactionAnswer).when(mockedRetryingTransactionHelper).doInTransaction(any(RetryingTransactionCallback.class), + Matchers.anyBoolean(), Matchers.anyBoolean()); + // setup data List dispositionActions = buildList(CUTOFF, RETAIN); executer.setDispositionActions(dispositionActions); + executer.setBatchSize(1); // setup interactions doReturn(mockedResultSet).when(mockedSearchService).query(any(SearchParameters.class)); @@ -104,12 +121,21 @@ public class DispositionLifecycleJobExecuterUnitTest extends BaseUnitTest * Helper method to verify that the query has been executed and closed */ private void verifyQuery() + { + verifyQueryTimes(1); + } + + /** + * Helper method to verify that the query has been executed and closed + * @param numberOfInvocation number of times the query has been executed and closed + */ + private void verifyQueryTimes(int numberOfInvocation) { ArgumentCaptor paramsCaptor = ArgumentCaptor.forClass(SearchParameters.class); - verify(mockedSearchService, times(1)).query(paramsCaptor.capture()); + verify(mockedSearchService, times(numberOfInvocation)).query(paramsCaptor.capture()); assertTrue(paramsCaptor.getValue().getQuery().contains(QUERY)); - verify(mockedResultSet, times(1)).getNodeRefs(); - verify(mockedResultSet, times(1)).close(); + verify(mockedResultSet, times(numberOfInvocation)).getNodeRefs(); + verify(mockedResultSet, times(numberOfInvocation)).close(); } /** @@ -143,24 +169,31 @@ public class DispositionLifecycleJobExecuterUnitTest extends BaseUnitTest // test data NodeRef node1 = generateNodeRef(); NodeRef node2 = generateNodeRef(); - List nodeRefs = buildList(node1, node2); // given - doReturn(nodeRefs).when(mockedResultSet).getNodeRefs(); doReturn(DESTROY).when(mockedNodeService).getProperty(node1, RecordsManagementModel.PROP_DISPOSITION_ACTION); doReturn(DESTROY).when(mockedNodeService).getProperty(node2, RecordsManagementModel.PROP_DISPOSITION_ACTION); + when(mockedResultSet.getNodeRefs()) + .thenReturn(buildList(node1)) + .thenReturn(buildList(node2)); + + when(mockedResultSet.hasMore()) + .thenReturn(true) + .thenReturn(false); + // when executer.executeImpl(); // then // ensure the query is executed and closed - verifyQuery(); + verifyQueryTimes(2); // ensure work is executed in transaction for each node processed verify(mockedNodeService, times(2)).exists(any(NodeRef.class)); - verify(mockedRetryingTransactionHelper, times(2)).doInTransaction(any(RetryingTransactionCallback.class)); + verify(mockedRetryingTransactionHelper, times(2)).doInTransaction(any(RetryingTransactionCallback.class), + Matchers.anyBoolean(), Matchers.anyBoolean()); // ensure each node is process correctly verify(mockedNodeService, times(1)).getProperty(node1, RecordsManagementModel.PROP_DISPOSITION_ACTION); @@ -198,7 +231,7 @@ public class DispositionLifecycleJobExecuterUnitTest extends BaseUnitTest // ensure no more interactions verifyNoMoreInteractions(mockedNodeService); - verifyZeroInteractions(mockedRecordsManagementActionService, mockedRetryingTransactionHelper); + verifyZeroInteractions(mockedRecordsManagementActionService); } /** @@ -211,27 +244,33 @@ public class DispositionLifecycleJobExecuterUnitTest extends BaseUnitTest // test data NodeRef node1 = generateNodeRef(); NodeRef node2 = generateNodeRef(); - List nodeRefs = buildList(node1, node2); NodeRef parent = generateNodeRef(); ChildAssociationRef parentAssoc = new ChildAssociationRef(ASSOC_NEXT_DISPOSITION_ACTION, parent, generateQName(), generateNodeRef()); - // given - doReturn(nodeRefs).when(mockedResultSet).getNodeRefs(); doReturn(CUTOFF).when(mockedNodeService).getProperty(node1, RecordsManagementModel.PROP_DISPOSITION_ACTION); doReturn(RETAIN).when(mockedNodeService).getProperty(node2, RecordsManagementModel.PROP_DISPOSITION_ACTION); doReturn(parentAssoc).when(mockedNodeService).getPrimaryParent(any(NodeRef.class)); + when(mockedResultSet.getNodeRefs()) + .thenReturn(buildList(node1)) + .thenReturn(buildList(node2)); + + when(mockedResultSet.hasMore()) + .thenReturn(true) + .thenReturn(false); + // when executer.executeImpl(); // then // ensure the query is executed and closed - verifyQuery(); + verifyQueryTimes(2); // ensure work is executed in transaction for each node processed verify(mockedNodeService, times(2)).exists(any(NodeRef.class)); - verify(mockedRetryingTransactionHelper, times(2)).doInTransaction(any(RetryingTransactionCallback.class)); + verify(mockedRetryingTransactionHelper, times(2)).doInTransaction(any(RetryingTransactionCallback.class), + Matchers.anyBoolean(), Matchers.anyBoolean()); // ensure each node is process correctly // node1 From 6e268330da238cb815a35a15ca63c3a2c9212ad6 Mon Sep 17 00:00:00 2001 From: Epure Alexandru-Eusebiu Date: Wed, 7 Apr 2021 13:24:42 +0300 Subject: [PATCH 2/2] Remove verifyQuery method and use verifyQueryTimes(1) instead Remove unused import Update license header year to 2021 Clear the code a little bit by: Remove explicit type arguments Make use of lambda --- .../job/DispositionLifecycleJobExecuter.java | 2 +- ...spositionLifecycleJobExecuterUnitTest.java | 79 +++++++------------ 2 files changed, 30 insertions(+), 51 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuter.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuter.java index b44cf229d3..242bb63480 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuter.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuter.java @@ -2,7 +2,7 @@ * #%L * Alfresco Records Management Module * %% - * Copyright (C) 2005 - 2020 Alfresco Software Limited + * Copyright (C) 2005 - 2021 Alfresco Software Limited * %% * This file is part of the Alfresco software. * - diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuterUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuterUnitTest.java index 30b3f82ee7..035997bf26 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuterUnitTest.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/job/DispositionLifecycleJobExecuterUnitTest.java @@ -2,7 +2,7 @@ * #%L * Alfresco Records Management Module * %% - * Copyright (C) 2005 - 2020 Alfresco Software Limited + * Copyright (C) 2005 - 2021 Alfresco Software Limited * %% * This file is part of the Alfresco software. * - @@ -59,7 +59,6 @@ import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Matchers; import org.mockito.Mock; -import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; /** @@ -75,7 +74,7 @@ public class DispositionLifecycleJobExecuterUnitTest extends BaseUnitTest private static final String RETAIN = "retain"; private static final String DESTROY = "destroy"; - /** test query snipit */ + /** test query snippet */ private static final String QUERY = "\"" + CUTOFF + "\" OR \"" + RETAIN + "\""; /** mocked result set */ @@ -94,17 +93,11 @@ public class DispositionLifecycleJobExecuterUnitTest extends BaseUnitTest super.before(); // Because of the fix implemented in MNT-22310, a new setup for retrying transaction helper is required. - Answer doInTransactionAnswer = new Answer() - { - @SuppressWarnings("rawtypes") - @Override - public Object answer(InvocationOnMock invocation) throws Throwable - { - RetryingTransactionCallback callback = (RetryingTransactionCallback)invocation.getArguments()[0]; - return callback.execute(); - } + Answer doInTransactionAnswer = invocation -> { + RetryingTransactionCallback callback = (RetryingTransactionCallback)invocation.getArguments()[0]; + return callback.execute(); }; - doAnswer(doInTransactionAnswer).when(mockedRetryingTransactionHelper).doInTransaction(any(RetryingTransactionCallback.class), + doAnswer(doInTransactionAnswer).when(mockedRetryingTransactionHelper).doInTransaction(any(RetryingTransactionCallback.class), Matchers.anyBoolean(), Matchers.anyBoolean()); // setup data @@ -117,14 +110,6 @@ public class DispositionLifecycleJobExecuterUnitTest extends BaseUnitTest when(mockedResultSet.hasMore()).thenReturn(false); } - /** - * Helper method to verify that the query has been executed and closed - */ - private void verifyQuery() - { - verifyQueryTimes(1); - } - /** * Helper method to verify that the query has been executed and closed * @param numberOfInvocation number of times the query has been executed and closed @@ -153,7 +138,7 @@ public class DispositionLifecycleJobExecuterUnitTest extends BaseUnitTest // then // ensure the query is executed and closed - verifyQuery(); + verifyQueryTimes(1); // ensure nothing else happens becuase we have no results verifyZeroInteractions(mockedNodeService, mockedRecordFolderService, mockedRetryingTransactionHelper); @@ -192,7 +177,7 @@ public class DispositionLifecycleJobExecuterUnitTest extends BaseUnitTest // ensure work is executed in transaction for each node processed verify(mockedNodeService, times(2)).exists(any(NodeRef.class)); - verify(mockedRetryingTransactionHelper, times(2)).doInTransaction(any(RetryingTransactionCallback.class), + verify(mockedRetryingTransactionHelper, times(2)).doInTransaction(any(RetryingTransactionCallback.class), Matchers.anyBoolean(), Matchers.anyBoolean()); // ensure each node is process correctly @@ -224,7 +209,7 @@ public class DispositionLifecycleJobExecuterUnitTest extends BaseUnitTest // then // ensure the query is executed and closed - verifyQuery(); + verifyQueryTimes(1); // ensure the node exist check is made for the node verify(mockedNodeService, times(1)).exists(any(NodeRef.class)); @@ -269,7 +254,7 @@ public class DispositionLifecycleJobExecuterUnitTest extends BaseUnitTest // ensure work is executed in transaction for each node processed verify(mockedNodeService, times(2)).exists(any(NodeRef.class)); - verify(mockedRetryingTransactionHelper, times(2)).doInTransaction(any(RetryingTransactionCallback.class), + verify(mockedRetryingTransactionHelper, times(2)).doInTransaction(any(RetryingTransactionCallback.class), Matchers.anyBoolean(), Matchers.anyBoolean()); // ensure each node is process correctly @@ -318,32 +303,26 @@ public class DispositionLifecycleJobExecuterUnitTest extends BaseUnitTest final NodeRef node4 = generateNodeRef(); // mock the search service to return the right page - when(mockedSearchService.query(any(SearchParameters.class))).thenAnswer( - new Answer() + when(mockedSearchService.query(any(SearchParameters.class))).thenAnswer((Answer) invocation -> { + SearchParameters params = invocation.getArgumentAt(0, SearchParameters.class); + if (params.getSkipCount() == 0) { - @Override - public ResultSet answer(InvocationOnMock invocation) - { - SearchParameters params = invocation.getArgumentAt(0, SearchParameters.class); - if (params.getSkipCount() == 0) - { - // mock first page - ResultSet result1 = mock(ResultSet.class); - when(result1.getNodeRefs()).thenReturn(Arrays.asList(node1, node2)); - when(result1.hasMore()).thenReturn(true); - return result1; - } - else if (params.getSkipCount() == 2) - { - // mock second page - ResultSet result2 = mock(ResultSet.class); - when(result2.getNodeRefs()).thenReturn(Arrays.asList(node3, node4)); - when(result2.hasMore()).thenReturn(false); - return result2; - } - throw new IndexOutOfBoundsException("Pagination did not stop after the second page!"); - } - }); + // mock first page + ResultSet result1 = mock(ResultSet.class); + when(result1.getNodeRefs()).thenReturn(Arrays.asList(node1, node2)); + when(result1.hasMore()).thenReturn(true); + return result1; + } + else if (params.getSkipCount() == 2) + { + // mock second page + ResultSet result2 = mock(ResultSet.class); + when(result2.getNodeRefs()).thenReturn(Arrays.asList(node3, node4)); + when(result2.hasMore()).thenReturn(false); + return result2; + } + throw new IndexOutOfBoundsException("Pagination did not stop after the second page!"); + }); // call the service executer.executeImpl();