From a158f3bd3d4c6b882d5e2f6b3e299767b6f7848d Mon Sep 17 00:00:00 2001 From: Pavol Mederly Date: Fri, 7 May 2021 16:52:48 +0200 Subject: [PATCH] Avoid double-loading of resource objects During recomputation the resource objects were (fully) loaded twice in succession - during initial context load. This commit eliminates that. This resolves MID-7041. --- .../schema/GetOperationOptionsBuilder.java | 2 + .../GetOperationOptionsBuilderImpl.java | 10 +++ .../impl/lens/projector/ContextLoader.java | 70 ++++++++++++------- .../model/intest/orgstruct/TestOrgStruct.java | 4 +- 4 files changed, 57 insertions(+), 29 deletions(-) diff --git a/infra/schema/src/main/java/com/evolveum/midpoint/schema/GetOperationOptionsBuilder.java b/infra/schema/src/main/java/com/evolveum/midpoint/schema/GetOperationOptionsBuilder.java index c05933929e2..f693b576298 100644 --- a/infra/schema/src/main/java/com/evolveum/midpoint/schema/GetOperationOptionsBuilder.java +++ b/infra/schema/src/main/java/com/evolveum/midpoint/schema/GetOperationOptionsBuilder.java @@ -52,6 +52,8 @@ public interface GetOperationOptionsBuilder { GetOperationOptionsBuilder staleness(Long value); GetOperationOptionsBuilder forceRefresh(); GetOperationOptionsBuilder forceRefresh(Boolean value); + GetOperationOptionsBuilder forceRetry(); + GetOperationOptionsBuilder forceRetry(Boolean value); GetOperationOptionsBuilder distinct(); GetOperationOptionsBuilder distinct(Boolean value); GetOperationOptionsBuilder attachDiagData(); diff --git a/infra/schema/src/main/java/com/evolveum/midpoint/schema/GetOperationOptionsBuilderImpl.java b/infra/schema/src/main/java/com/evolveum/midpoint/schema/GetOperationOptionsBuilderImpl.java index 9c693e13ec2..5c3efcdbb00 100644 --- a/infra/schema/src/main/java/com/evolveum/midpoint/schema/GetOperationOptionsBuilderImpl.java +++ b/infra/schema/src/main/java/com/evolveum/midpoint/schema/GetOperationOptionsBuilderImpl.java @@ -201,6 +201,16 @@ public GetOperationOptionsBuilder forceRefresh(Boolean value) { return forPaths(opts -> opts.setForceRefresh(value)); } + @Override + public GetOperationOptionsBuilder forceRetry() { + return forceRetry(true); + } + + @Override + public GetOperationOptionsBuilder forceRetry(Boolean value) { + return forPaths(opts -> opts.setForceRetry(value)); + } + @Override public GetOperationOptionsBuilder distinct() { return distinct(true); diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/projector/ContextLoader.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/projector/ContextLoader.java index 61f55e78421..16086675abf 100644 --- a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/projector/ContextLoader.java +++ b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/projector/ContextLoader.java @@ -666,9 +666,9 @@ private void loadLinkRefsFromFocus(LensContext context, LOGGER.trace("Inactive linkRef: will only refresh it, no processing. Relation={}, ref={}", linkRefVal.getRelation(), linkRefVal); // We do this just to restore old behavior that ensures that linked shadows are quick-refreshed, - // deleting e.g. expired pending operations (see TestMultiResource.test429). - var options = createGetOptions(context); - refreshLinkedShadow(oid, options, task, result); + // deleting e.g. expired pending operations (see TestMultiResource.test429). But only if full reconciliation + // is requested. + refreshLinkedShadow(oid, context, task, result); continue; } if (StringUtils.isBlank(oid)) { @@ -686,7 +686,7 @@ private void loadLinkRefsFromFocus(LensContext context, //noinspection unchecked PrismObject shadowFromLink = linkRefVal.getObject(); if (shadowFromLink == null) { - var options = createGetOptions(context); + var options = createStandardGetOptions(); LOGGER.trace("Loading shadow {} from linkRef, options={}", oid, options); try { shadow = provisioningService.getObject(ShadowType.class, oid, options, task, result); @@ -726,22 +726,27 @@ private void loadLinkRefsFromFocus(LensContext context, } } - private Collection> createGetOptions(LensContext context) { - GetOperationOptions rootOpts; + private Collection> createStandardGetOptions() { + // Using NO_FETCH so we avoid reading in a full account. This is more efficient as we don't need full account here. + // We need to fetch from provisioning and not repository so the correct definition will be set. + return SchemaService.get().getOperationOptionsBuilder() + .noFetch() + .pointInTime(PointInTimeType.FUTURE) + .build(); + } + + private void refreshLinkedShadow(String oid, LensContext context, Task task, OperationResult result) { + Collection> options; if (context.isDoReconciliationForAllProjections()) { - rootOpts = GetOperationOptions.createForceRetry(); + options = SchemaService.get().getOperationOptionsBuilder() + .forceRetry() + .build(); } else { - // Using NO_FETCH so we avoid reading in a full account. This is more efficient as we don't need full account here. - // We need to fetch from provisioning and not repository so the correct definition will be set. - rootOpts = GetOperationOptions.createNoFetch(); - rootOpts.setPointInTimeType(PointInTimeType.FUTURE); + options = createStandardGetOptions(); } - return SelectorOptions.createCollection(rootOpts); - } - - private void refreshLinkedShadow(String oid, Collection> options, Task task, - OperationResult result) { try { + // TODO should we even do this if no reconciliation is requested? + // We call it with noFetch and we even ignore any exceptions. provisioningService.getObject(ShadowType.class, oid, options, task, result); } catch (Exception e) { result.muteLastSubresultError(); @@ -1262,17 +1267,7 @@ private void finishLoadOfProjectionContext(LensContext "Projection "+projContext.getHumanReadableName()+" with null OID, no representation and no resource OID in account sync context "+projContext); } } else { - GetOperationOptions rootOptions = GetOperationOptions.createPointInTimeType(PointInTimeType.FUTURE); - if (projContext.isDoReconciliation()) { - rootOptions.setForceRefresh(true); - if (SchemaConstants.CHANNEL_DISCOVERY_URI.equals(context.getChannel())) { - // Avoid discovery loops - rootOptions.setDoNotDiscovery(true); - } - } else { - rootOptions.setNoFetch(true); - } - rootOptions.setAllowNotFound(true); + GetOperationOptions rootOptions = createRootOptions(context, projContext); Collection> options = SelectorOptions.createCollection(rootOptions); LOGGER.trace("Loading shadow {} for projection {}, options={}", projectionObjectOid, projContext.getHumanReadableName(), options); @@ -1430,6 +1425,27 @@ private void finishLoadOfProjectionContext(LensContext setPrimaryDeltaOldValue(projContext); } + @NotNull + private GetOperationOptions createRootOptions(LensContext context, + LensProjectionContext projContext) { + GetOperationOptions rootOptions = GetOperationOptions.createPointInTimeType(PointInTimeType.FUTURE); + if (context.isDoReconciliationForAllProjections()) { + // Not sure why exactly in this way, but this is copied from existing code. + rootOptions.setForceRetry(true); + } + if (projContext.isDoReconciliation() || context.isDoReconciliationForAllProjections()) { + rootOptions.setForceRefresh(true); + if (SchemaConstants.CHANNEL_DISCOVERY_URI.equals(context.getChannel())) { + // Avoid discovery loops + rootOptions.setDoNotDiscovery(true); + } + } else { + rootOptions.setNoFetch(true); + } + rootOptions.setAllowNotFound(true); + return rootOptions; + } + private boolean needToReload(LensContext context, LensProjectionContext projContext) { ResourceShadowDiscriminator discr = projContext.getResourceShadowDiscriminator(); diff --git a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/orgstruct/TestOrgStruct.java b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/orgstruct/TestOrgStruct.java index 663f2ea5cfd..959c770abae 100644 --- a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/orgstruct/TestOrgStruct.java +++ b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/orgstruct/TestOrgStruct.java @@ -552,8 +552,8 @@ public void test232JackDestroyRefsAndRecompute() throws Exception { // Why so many operations? But this is a very special case. As long as we do not see significant // increase of operation count in normal scenarios we are quite OK. - assertCounterIncrement(InternalCounters.SHADOW_FETCH_OPERATION_COUNT, 4); - assertCounterIncrement(InternalCounters.CONNECTOR_OPERATION_COUNT, 8); + assertCounterIncrement(InternalCounters.SHADOW_FETCH_OPERATION_COUNT, 2); + assertCounterIncrement(InternalCounters.CONNECTOR_OPERATION_COUNT, 5); } /**