Remove unlimited collections from the repository API #496

kaizimmerm · 2017-04-25T11:21:17Z

hawkBit in many areas of the repository returns unpaged and unlimited collections. This has a risk as the system will be asked to query huge collections of data in certain scenarios. Query 500k entities in one query for sure will have negative impact on the overall system. I can't even say what will explode first (persistence, Java Heap, APIs...).

Examples on the entity APIs itself are:

Action: List getActionStatus();
DistributionSet: Set getTags();

Examples on repository APIs:

DeploymentManagement: List findActiveActionsByTarget(@notempty String controllerId);
TargetManagement: List findTargetsByTag(@notempty String tagName);

Well, the risk is different depending on the use case. While active actions on a target should in most cases be a two digit number max it is not actually guaranteed. All targets assigned to a tag however might easily go in the 6 digit direction.

I fixed most scenarios by means of providing or using repository APIs with limits. There are some left. Here, however I believe we should introduce configurable quotas (e.g. how many software modules can be in a distribution set). Until then the risk stays in the system.

However, unfortunately on the public APIs we are propagating unpaged access as well (targets or DS assigned to a tag):

I created new resources with proper paging and restricted the other to 500 entities max and declared them deprecated.
One API call (remove all targets/DSs from a tag) could not be fixed. I removed it. Technically an API break but I think the use case of that resource is highly questionable (better delete the tag and create a new one).
While looking for tests that have to be aligned I figured that tests for tag resources in mgmt API where completely missing. I added those as well.

On the way I fixed a few typos in events and provided a proper API for the quotas we already have in the system. Should be quite easy to extend those in the future.

Note: this is another task of #197

Reviewers: @schabdo @sirwayne

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Conflicts: hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiCancelActionTest.java hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiDeploymentBaseTest.java hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/autoassign/AutoAssignScheduler.java hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/rollout/RolloutScheduler.java hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/DeploymentManagementTest.java

Conflicts: hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/DistributionSetTagRepository.java hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaDistributionSetManagement.java hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaSystemManagement.java hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaTargetManagement.java hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/TargetTagRepository.java hawkbit-repository/hawkbit-repository-test/src/main/java/org/eclipse/hawkbit/repository/test/util/JpaTestRepositoryManagement.java

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Conflicts: hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/integration/AmqpMessageHandlerServiceIntegrationTest.java Signed-off-by: SirWayne <dennis.melzer@bosch-si.com>

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Conflicts: hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/integration/AmqpMessageHandlerServiceIntegrationTest.java hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/DistributionSetRepository.java

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Conflicts: hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/integration/AmqpMessageHandlerServiceIntegrationTest.java

MelleD · 2017-04-27T10:45:14Z

hawkbit-mgmt-api/src/main/java/org/eclipse/hawkbit/mgmt/rest/api/MgmtRestConstants.java

@@ -110,9 +120,14 @@
    public static final String TARGET_FILTER_V1_REQUEST_MAPPING = BASE_V1_REQUEST_MAPPING + "/targetfilters";

    /**
+     * The deprecated tag URL mapping rest resource.
+     */
+    public static final String DEPRECATED_DISTRIBUTIONSET_REQUEST_MAPPING = "/{distributionsetTagId}/distributionsets";


Add @Deprecated

MelleD · 2017-04-27T10:49:23Z

hawkbit-mgmt-resource/src/main/java/org/eclipse/hawkbit/mgmt/rest/resource/PagingUtility.java

+    static Sort sanitizeTagSortParam(final String sortParam) {
+        if (sortParam == null) {
+            // default
+            return new Sort(Direction.ASC, TargetFields.NAME.getFieldName());


Use TageFields.Name instead of Target

MelleD · 2017-04-27T11:07:34Z

...kbit-repository-core/src/main/java/org/eclipse/hawkbit/repository/StaticQuotaManagement.java

+
+import org.eclipse.hawkbit.security.HawkbitSecurityProperties;
+
+public class StaticQuotaManagement implements QuotaManagement {


Add javadoc

I would rename the class to PropertiesQuotaManagement, because you use properties for the quota

MelleD · 2017-04-27T11:13:20Z

...tory/hawkbit-repository-core/src/main/java/org/eclipse/hawkbit/repository/RolloutHelper.java

     */
-    public static void verifyRolloutGroupParameter(final int amountGroup) {
+    public static void verifyRolloutGroupParameter(final int amountGroup, final int maxGroups) {


In my opinion it's better to make a real bean and inject the QuotaManagement here. Because now every developer can set another maxGroups constraint for every call.

MelleD · 2017-04-27T11:15:11Z

...bit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaTargetManagement.java

-        if (!tag.isPresent()) {
-            return Collections.emptyList();
+    public Page<Target> findTargetsByTag(final Pageable pageable, final Long tagId) {
+        if (!targetTagRepository.exists(tagId)) {


create a private method to avoid duplicate code

Conflicts: hawkbit-dmf-parent/hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/integration/AmqpMessageHandlerServiceIntegrationTest.java

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

kaizimmerm · 2017-04-28T07:20:30Z

@sirwayne I like your ideas. All implemented.

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Conflicts: hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiArtifactDownloadTest.java

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

schabdo · 2017-05-04T15:19:40Z

...-simulator/src/main/java/org/eclipse/hawkbit/mgmt/client/scenarios/ConfigurableScenario.java

@@ -316,31 +316,38 @@ private void waitUntilRolloutIsReady(final Long id) {

    private void createDistributionSets(final Scenario scenario) {
        LOGGER.info("Creating {} distribution sets", scenario.getDistributionSets());
+        final int pages = (scenario.getDistributionSets() / PAGE_SIZE) >= 1


The page calculation is broken when you slightly exceed the page size. For example: 506 or 501 distribution sets will result in a page size of 1 so in the end this code creates only 500 entities and not the expected amount.

Same happens to the other entities e.g. targets so I won't comment it there as well

I'm afraid it is still broken. Use BigDecimal instead if int or float for the calculation.

Example:
new BigDecimal(1002).divide(new BigDecimal(500), BigDecimal.ROUND_UP) => 3

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

schabdo · 2017-05-04T16:18:25Z

hawkbit-mgmt-resource/src/main/java/org/eclipse/hawkbit/mgmt/rest/resource/MgmtTagMapper.java

-                .withRel("assignedTargets"));
+        response.add(linkTo(methodOn(MgmtTargetTagRestApi.class).getAssignedTargets(targetTag.getId(),
+                Integer.parseInt(MgmtRestConstants.REQUEST_PARAMETER_PAGING_DEFAULT_OFFSET),
+                Integer.parseInt(MgmtRestConstants.REQUEST_PARAMETER_PAGING_DEFAULT_LIMIT), null, null))


There is an int value already available: Use MgmtRestConstants.REQUEST_PARAMETER_PAGING_DEFAULT_LIMIT_VALUE instead of parsing the string

Interesting. I did not know that we have this value constant. I fixed this on all the mappers and created one for the offset as well.

schabdo · 2017-05-04T16:19:58Z

hawkbit-mgmt-resource/src/main/java/org/eclipse/hawkbit/mgmt/rest/resource/MgmtTagMapper.java

-                methodOn(MgmtDistributionSetTagRestApi.class).getAssignedDistributionSets(distributionSetTag.getId()))
+        response.add(linkTo(methodOn(MgmtDistributionSetTagRestApi.class).getAssignedDistributionSets(
+                distributionSetTag.getId(), Integer.parseInt(MgmtRestConstants.REQUEST_PARAMETER_PAGING_DEFAULT_OFFSET),
+                Integer.parseInt(MgmtRestConstants.REQUEST_PARAMETER_PAGING_DEFAULT_LIMIT), null, null))


schabdo · 2017-05-04T17:12:28Z

...t-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/RolloutManagementTest.java

-            // OK
-        }
-
+        assertThatExceptionOfType(EntityAlreadyExistsException.class)


schabdo · 2017-05-04T17:16:58Z

...test/src/main/java/org/eclipse/hawkbit/repository/test/util/JpaTestRepositoryManagement.java


 public class JpaTestRepositoryManagement implements TestRepositoryManagement {

    private static final Logger LOGGER = LoggerFactory.getLogger(JpaTestRepositoryManagement.class);
+    private static final Pageable pageReq = new PageRequest(0, 400, new Sort(Direction.ASC, "id"));


Use CAPITAL LETTERS for variable pageReq => it is a constant

Copied from abstract integration test :) I will fix this in both classes.

schabdo · 2017-05-04T17:18:34Z

...-repository-test/src/main/java/org/eclipse/hawkbit/repository/test/util/TestdataFactory.java

+     */
+    public List<DistributionSet> createDistributionSetsWithoutModules(final int number) {
+
+        final List<DistributionSet> sets = new ArrayList<>();


Set size of list => new ArrayList<>(number)

That does not help much. Lists.newArrayListWithExpectedSize(number) is the right one. But you are generally right of course.

schabdo · 2017-05-04T17:24:54Z

hawkbit-rest-core/src/test/java/org/eclipse/hawkbit/rest/util/JsonBuilder.java

+            try {
+                createTagLine(builder, tag);
+            } catch (final Exception e) {
+                e.printStackTrace();


Why not throwing an exception here? It's obviously wrong JSON. It might be easier to find when you stop processing the tags here instead of waiting for the test to fail due to missing tags later

Well, the code is copied from the other builders but I actually don't know why the exception is catched. I will fix this in the entire class.

Conflicts: hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/management/dstable/DistributionTable.java hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/management/dstable/DistributionTableLayout.java hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/rollout/rollout/DefineGroupsLayout.java

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Remove unlimited collections from the repository API (eclipse-hawkbit#496)

…#496) * Started to get rid of unlimited collections Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Align API usage. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * fix compile issues. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Fix tests. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Remove comments Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Performance optimizations. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Remove dead code. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Allign method names Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Wait until the action update event is processed Conflicts: hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/integration/AmqpMessageHandlerServiceIntegrationTest.java Signed-off-by: SirWayne <dennis.melzer@bosch-si.com> * Started new tag APIs Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Quotas into central interface. Tag tests added. Event names fixed. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Simplified consumer run for every tenant. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * remove unused fields. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Alligned beans. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Deprecated client methods for old resources. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Fix new foreach method. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Fix transaction for foreach. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Extended DS creating to handle larger volumes. Fix on Readme. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Fixed simulator bug and cleaned up tests. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Fix in sorting. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Remove configuration processor. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Fix wrong usage of sanitize. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Missing brackets. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Fix README API compatability. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Fix misinterpretation of pessimistic locking exceptions. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Fix stability sentence. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Code cleanup. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Fixed page calculation Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

kaizimmerm and others added 24 commits March 31, 2017 17:36

Started to get rid of unlimited collections

0dde869

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Align API usage.

d689140

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Merge branch 'master' of local repository into fix_memory_repo

6e7d12a

fix compile issues.

943d8be

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Fix tests.

fb8f4a9

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Remove comments

2824981

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Performance optimizations.

fb807c2

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Remove dead code.

c725279

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Allign method names

7846baf

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Wait until the action update event is processed

f3c3d0a

Conflicts: hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/integration/AmqpMessageHandlerServiceIntegrationTest.java Signed-off-by: SirWayne <dennis.melzer@bosch-si.com>

Started new tag APIs

d003910

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Merge branch 'master' into fix_memory_repo

bdb5b0a

Conflicts: hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/integration/AmqpMessageHandlerServiceIntegrationTest.java hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/DistributionSetRepository.java

Quotas into central interface. Tag tests added. Event names fixed.

9739106

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Simplified consumer run for every tenant.

5455c3a

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

remove unused fields.

709b4aa

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Alligned beans.

c41640d

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Deprecated client methods for old resources.

7c1d89f

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Fix new foreach method.

34f0bef

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Fix transaction for foreach.

99cae2f

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Extended DS creating to handle larger volumes. Fix on Readme.

4310145

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Fixed simulator bug and cleaned up tests.

eed965e

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Merge branch 'master' into fix_memory_repo

058a725

Conflicts: hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/integration/AmqpMessageHandlerServiceIntegrationTest.java

MelleD reviewed Apr 27, 2017

View reviewed changes

Merge branch 'master' into fix_memory_repo

c4c0126

Conflicts: hawkbit-dmf-parent/hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/integration/AmqpMessageHandlerServiceIntegrationTest.java

Fix in sorting.

0088564

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

kaizimmerm added 5 commits April 28, 2017 09:23

Remove configuration processor.

fe2770f

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Fix wrong usage of sanitize.

38536d4

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Missing brackets.

23e2bfc

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Fix README API compatability.

1b610ec

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Merge branch 'master' into fix_memory_repo

972f1d2

Conflicts: hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiArtifactDownloadTest.java

MelleD approved these changes May 3, 2017

View reviewed changes

Fix misinterpretation of pessimistic locking exceptions.

22a66ab

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

schabdo reviewed May 4, 2017

View reviewed changes

Fix stability sentence.

693eede

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

schabdo reviewed May 4, 2017

View reviewed changes

kaizimmerm added 3 commits May 6, 2017 08:52

Code cleanup.

53ded7f

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

Fixed page calculation

e513fdd

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>

kaizimmerm merged commit c18e9f5 into eclipse-hawkbit:master May 9, 2017

kaizimmerm deleted the fix_memory_repo branch May 9, 2017 14:40

kinbod added a commit to kinbod/hawkbit that referenced this pull request May 10, 2017

Merge pull request #2 from eclipse/master

b20eea8

Remove unlimited collections from the repository API (eclipse-hawkbit#496)

singh-himanshu mentioned this pull request May 12, 2017

DDI API: Extend API response to report previous update execution status. #506

Merged

kaizimmerm added this to the 0.2.0M4 milestone Feb 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unlimited collections from the repository API #496

Remove unlimited collections from the repository API #496

kaizimmerm commented Apr 25, 2017 •

edited

Loading

MelleD Apr 27, 2017

MelleD Apr 27, 2017

MelleD Apr 27, 2017

MelleD Apr 27, 2017

MelleD Apr 27, 2017

kaizimmerm commented Apr 28, 2017 •

edited

Loading

schabdo May 4, 2017

kaizimmerm May 6, 2017

schabdo May 8, 2017

schabdo May 4, 2017

kaizimmerm May 6, 2017

schabdo May 4, 2017

schabdo May 4, 2017

schabdo May 4, 2017

kaizimmerm May 6, 2017

schabdo May 4, 2017

kaizimmerm May 6, 2017

schabdo May 4, 2017

kaizimmerm May 6, 2017 •

edited

Loading


		import org.eclipse.hawkbit.security.HawkbitSecurityProperties;

		public class StaticQuotaManagement implements QuotaManagement {

Remove unlimited collections from the repository API #496

Remove unlimited collections from the repository API #496

Conversation

kaizimmerm commented Apr 25, 2017 • edited Loading

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

kaizimmerm commented Apr 28, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaizimmerm May 6, 2017 • edited Loading

Choose a reason for hiding this comment

kaizimmerm commented Apr 25, 2017 •

edited

Loading

kaizimmerm commented Apr 28, 2017 •

edited

Loading

kaizimmerm May 6, 2017 •

edited

Loading