From 494170405a03e366da57336fbd7e8b3605f79bb5 Mon Sep 17 00:00:00 2001 From: Avgustin Marinov Date: Wed, 26 Jun 2024 11:06:56 +0300 Subject: [PATCH] Small Rollout create code improvements (#1754) Signed-off-by: Marinov Avgustin --- .../hawkbit/repository/RolloutHelper.java | 73 ++++++------------- .../repository/jpa/JpaRolloutExecutor.java | 2 +- .../jpa/management/JpaRolloutManagement.java | 13 +++- .../management/RolloutManagementFlowTest.java | 9 +-- 4 files changed, 39 insertions(+), 58 deletions(-) diff --git a/hawkbit-repository/hawkbit-repository-core/src/main/java/org/eclipse/hawkbit/repository/RolloutHelper.java b/hawkbit-repository/hawkbit-repository-core/src/main/java/org/eclipse/hawkbit/repository/RolloutHelper.java index 5c76be5c8..e1a7efac5 100644 --- a/hawkbit-repository/hawkbit-repository-core/src/main/java/org/eclipse/hawkbit/repository/RolloutHelper.java +++ b/hawkbit-repository/hawkbit-repository-core/src/main/java/org/eclipse/hawkbit/repository/RolloutHelper.java @@ -34,8 +34,7 @@ public final class RolloutHelper { /** * Verifies that the required success condition and action are actually set. * - * @param conditions - * input conditions and actions + * @param conditions input conditions and actions */ public static void verifyRolloutGroupConditions(final RolloutGroupConditions conditions) { if (conditions.getSuccessCondition() == null) { @@ -50,8 +49,7 @@ public final class RolloutHelper { * Verifies that the group has the required success condition and action and a * valid target percentage. * - * @param group - * the input group + * @param group the input group * @return the verified group */ public static RolloutGroup verifyRolloutGroupHasConditions(final RolloutGroup group) { @@ -71,26 +69,20 @@ public final class RolloutHelper { /** * Verify if the supplied amount of groups is in range * - * @param amountGroup - * amount of groups - * @param quotaManagement - * to retrieve maximum number of groups allowed + * @param amountGroup amount of groups + * @param quotaManagement to retrieve maximum number of groups allowed */ - public static void verifyRolloutGroupParameter(final int amountGroup, final QuotaManagement quotaManagement) { - if (amountGroup <= 0) { - throw new ValidationException("The amount of groups cannot be lower than or equal to zero for static rollouts"); - } else if (amountGroup > quotaManagement.getMaxRolloutGroupsPerRollout()) { + public static void verifyRolloutGroupAmount(final int amountGroup, final QuotaManagement quotaManagement) { + if (amountGroup > quotaManagement.getMaxRolloutGroupsPerRollout()) { throw new AssignmentQuotaExceededException( "The amount of groups cannot be greater than " + quotaManagement.getMaxRolloutGroupsPerRollout()); - } } /** * Verify that the supplied percentage is in range * - * @param percentage - * the percentage + * @param percentage the percentage */ public static void verifyRolloutGroupTargetPercentage(final float percentage) { if (percentage <= 0) { @@ -104,8 +96,7 @@ public final class RolloutHelper { * Modifies the target filter query to only match targets that were created * after the Rollout. * - * @param rollout - * Rollout to derive the filter from + * @param rollout Rollout to derive the filter from * @return resulting target filter query */ public static String getTargetFilterQuery(final Rollout rollout) { @@ -113,12 +104,9 @@ public final class RolloutHelper { } /** - * @param targetFilter - * the target filter tp be extended - * @param createdAt - * timestamp - * @return a target filter query that only matches targets that were created - * after the provided timestamp. + * @param targetFilter the target filter tp be extended + * @param createdAt timestamp + * @return a target filter query that only matches targets that were created after the provided timestamp. */ public static String getTargetFilterQuery(final String targetFilter, final Long createdAt) { if (createdAt != null) { @@ -130,10 +118,8 @@ public final class RolloutHelper { /** * Verifies that the Rollout is in the required status. * - * @param rollout - * the Rollout - * @param status - * the Status + * @param rollout the Rollout + * @param status the Status */ public static void verifyRolloutInStatus(final Rollout rollout, final Rollout.RolloutStatus status) { if (rollout.getStatus() != status) { @@ -145,10 +131,8 @@ public final class RolloutHelper { * Filters the groups of a Rollout to match a specific status and adds a group * to the result. * - * @param status - * the required status for the groups - * @param group - * the group to add + * @param status the required status for the groups + * @param group the group to add * @return list of groups */ public static List getGroupsByStatusIncludingGroup(final List groups, @@ -161,10 +145,8 @@ public final class RolloutHelper { * Creates an RSQL expression that matches all targets in the provided groups. * Links all target filter queries with OR. * - * @param groups - * the rollout groups - * @return RSQL string without base filter of the Rollout. Can be an empty - * string. + * @param groups the rollout groups + * @return RSQL string without base filter of the Rollout. Can be an empty string. */ public static String getAllGroupsTargetFilter(final List groups) { if (groups.stream().anyMatch(group -> StringUtils.isEmpty(group.getTargetFilterQuery()))) { @@ -179,14 +161,10 @@ public final class RolloutHelper { * Creates an RSQL Filter that matches all targets that are in the provided * group and in the provided groups. * - * @param baseFilter - * the base filter from the rollout - * @param groups - * the rollout groups - * @param group - * the target group - * @return RSQL string without base filter of the Rollout. Can be an empty - * string. + * @param baseFilter the base filter from the rollout + * @param groups the rollout groups + * @param group the target group + * @return RSQL string without base filter of the Rollout. Can be an empty string. */ public static String getOverlappingWithGroupsTargetFilter(final String baseFilter, final List groups, final RolloutGroup group) { @@ -220,10 +198,8 @@ public final class RolloutHelper { } /** - * @param baseFilter - * the base filter from the rollout - * @param group - * group for which the filter string should be created + * @param baseFilter the base filter from the rollout + * @param group group for which the filter string should be created * @return the final target filter query for a rollout group */ public static String getGroupTargetFilter(final String baseFilter, final RolloutGroup group) { @@ -236,8 +212,7 @@ public final class RolloutHelper { /** * Verifies that no targets are left * - * @param targetCount - * the count of left targets + * @param targetCount the count of left targets */ public static void verifyRemainingTargets(final long targetCount) { if (targetCount > 0) { diff --git a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutExecutor.java b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutExecutor.java index c6f70ec99..40416b84e 100644 --- a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutExecutor.java +++ b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutExecutor.java @@ -742,7 +742,7 @@ public class JpaRolloutExecutor implements RolloutExecutor { private void createDynamicGroup(final JpaRollout rollout, final RolloutGroup lastGroup, final int groupCount, final RolloutGroupStatus status) { try { - RolloutHelper.verifyRolloutGroupParameter(groupCount + 1, quotaManagement); + RolloutHelper.verifyRolloutGroupAmount(groupCount + 1, quotaManagement); } catch (final AssignmentQuotaExceededException e) { log.warn("Quota exceeded for dynamic rollout group creation: {}. Stop it", e.getMessage()); if (isRolloutComplete(rollout)) { diff --git a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaRolloutManagement.java b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaRolloutManagement.java index f4bcdeb46..965fdad8d 100644 --- a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaRolloutManagement.java +++ b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaRolloutManagement.java @@ -199,13 +199,15 @@ public class JpaRolloutManagement implements RolloutManagement { ConcurrencyFailureException.class }, maxAttempts = Constants.TX_RT_MAX, backoff = @Backoff(delay = Constants.TX_RT_DELAY)) public Rollout create(final RolloutCreate rollout, final int amountGroup, final boolean confirmationRequired, final RolloutGroupConditions conditions, final DynamicRolloutGroupTemplate dynamicRolloutGroupTemplate) { - if (amountGroup == 0) { + if (amountGroup < 0) { + throw new ValidationException("The amount of groups cannot be lower than or equal to zero for static rollouts"); + } else if (amountGroup == 0) { if (dynamicRolloutGroupTemplate == null) { throw new ValidationException( "When amount of groups is 0, the rollouts shall be dynamic and a dynamic group template must be provided"); } } else { - RolloutHelper.verifyRolloutGroupParameter(amountGroup, quotaManagement); + RolloutHelper.verifyRolloutGroupAmount(amountGroup, quotaManagement); } final JpaRollout rolloutRequest = (JpaRollout) rollout.build(); @@ -223,7 +225,10 @@ public class JpaRolloutManagement implements RolloutManagement { ConcurrencyFailureException.class }, maxAttempts = Constants.TX_RT_MAX, backoff = @Backoff(delay = Constants.TX_RT_DELAY)) public Rollout create(final RolloutCreate rollout, final List groups, final RolloutGroupConditions conditions) { - RolloutHelper.verifyRolloutGroupParameter(groups.size(), quotaManagement); + if (groups.isEmpty()) { + throw new ValidationException("The amount of groups cannot be 0"); + } + RolloutHelper.verifyRolloutGroupAmount(groups.size(), quotaManagement); final JpaRollout rolloutRequest = (JpaRollout) rollout.build(); final JpaRollout savedRollout = createRollout(rolloutRequest, false); return createRolloutGroups(groups, conditions, savedRollout); @@ -329,6 +334,8 @@ public class JpaRolloutManagement implements RolloutManagement { publishRolloutGroupCreatedEventAfterCommit(lastSavedGroup, rollout); } + // lastSavedGroup is never null! amountOfGroups > 0 (and has static groups) or dynamicRolloutGroupTemplate is + // not null (validated) and (validated) the rollout is dynamic, so has dynamic group rollout.setRolloutGroupsCreated(lastSavedGroup.isDynamic() ? amountOfGroups + 1 : amountOfGroups); return rolloutRepository.save(rollout); } diff --git a/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/RolloutManagementFlowTest.java b/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/RolloutManagementFlowTest.java index 57f63940c..d1ffc25bf 100644 --- a/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/RolloutManagementFlowTest.java +++ b/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/RolloutManagementFlowTest.java @@ -154,7 +154,7 @@ class RolloutManagementFlowTest extends AbstractJpaIntegrationTest { // filters for action of the last static group .filter(action -> Integer.parseInt(action.getTarget().getControllerId().substring(targetPrefix.length())) < amountGroups * 3) .forEach(this::finishAction); - executeWithoutOneTargetFromAGroup(rollout, dynamic1, 3); + executeWithoutOneTargetFromAGroup(dynamic1, rollout, 3); assertAndGetRunning(rollout, 1); // remains on in the first dynamic rolloutHandler.handleAll(); @@ -254,7 +254,7 @@ class RolloutManagementFlowTest extends AbstractJpaIntegrationTest { // filters for action of the last static group .filter(action -> Integer.parseInt(action.getTarget().getControllerId().substring(targetPrefix.length())) < amountGroups * 3) .forEach(this::finishAction); - executeWithoutOneTargetFromAGroup(rollout, dynamic1, 6); + executeWithoutOneTargetFromAGroup(dynamic1, rollout, 6); assertAndGetRunning(rollout, 1); // remains on in the first dynamic rolloutHandler.handleAll(); @@ -339,7 +339,7 @@ class RolloutManagementFlowTest extends AbstractJpaIntegrationTest { assertScheduled(rollout, 0); // executes dynamic1 without 1 target - executeWithoutOneTargetFromAGroup(rollout, dynamic1, 6); + executeWithoutOneTargetFromAGroup(dynamic1, rollout, 6); assertAndGetRunning(rollout, 1); // remains on in the first dynamic rolloutHandler.handleAll(); @@ -402,8 +402,7 @@ class RolloutManagementFlowTest extends AbstractJpaIntegrationTest { } } - private void executeWithoutOneTargetFromAGroup( - final Rollout rollout, final RolloutGroup group, final int count) { + private void executeWithoutOneTargetFromAGroup(final RolloutGroup group, final Rollout rollout, final int count) { // execute groups (without on of the last) assertThat(refresh(group).getStatus()).isEqualTo(RolloutGroupStatus.RUNNING); // skip on from the last group only