Remove inefficient saveListElementsBatch API 32/136932/7
authordanielhanrahan <daniel.hanrahan@est.tech>
Fri, 4 Aug 2023 10:22:43 +0000 (11:22 +0100)
committerdanielhanrahan <daniel.hanrahan@est.tech>
Tue, 9 Jan 2024 10:47:51 +0000 (10:47 +0000)
CpsDataService::saveListElementsBatch method is not needed as
saveListElements supports saving multiple list elements in a single
operation. This both simplifies implementation and greatly improves
performance when saving list elements, as the Yang parser need only
run once for the whole batch.

- Change InventoryPersistence to save CM-handles in batches of 100
  using existing CpsDataService::saveListElements method.
- Remove not needed CpsDataService::saveListElementBatch.

Issue-ID: CPS-2019
Signed-off-by: danielhanrahan <daniel.hanrahan@est.tech>
Change-Id: I8b74dda2917e094d064b42f2c0e4d57029b90395

cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/impl/inventory/InventoryPersistence.java
cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/impl/inventory/InventoryPersistenceImpl.java
cps-ncmp-service/src/test/groovy/org/onap/cps/ncmp/api/impl/inventory/InventoryPersistenceImplSpec.groovy
cps-ri/src/main/java/org/onap/cps/spi/impl/CpsDataPersistenceServiceImpl.java
cps-service/src/main/java/org/onap/cps/api/CpsDataService.java
cps-service/src/main/java/org/onap/cps/api/impl/CpsDataServiceImpl.java [changed mode: 0755->0644]
cps-service/src/main/java/org/onap/cps/spi/CpsDataPersistenceService.java
cps-service/src/test/groovy/org/onap/cps/api/impl/CpsDataServiceImplSpec.groovy
docs/release-notes.rst
integration-test/src/test/groovy/org/onap/cps/integration/functional/CpsDataServiceIntegrationSpec.groovy
integration-test/src/test/groovy/org/onap/cps/integration/performance/cps/WritePerfTest.groovy

index 09de9a7..9024eac 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  ============LICENSE_START=======================================================
- *  Copyright (C) 2022-2023 Nordix Foundation
+ *  Copyright (C) 2022-2024 Nordix Foundation
  *  Modifications Copyright (C) 2023 TechMahindra Ltd.
  *  ================================================================================
  *  Licensed under the Apache License, Version 2.0 (the "License");
@@ -22,6 +22,7 @@
 package org.onap.cps.ncmp.api.impl.inventory;
 
 import java.util.Collection;
+import java.util.List;
 import java.util.Map;
 import org.onap.cps.ncmp.api.impl.ncmppersistence.NcmpPersistence;
 import org.onap.cps.ncmp.api.impl.yangmodels.YangModelCmHandle;
@@ -98,7 +99,7 @@ public interface InventoryPersistence extends NcmpPersistence {
      *
      * @param yangModelCmHandles cm handle represented as Yang Models
      */
-    void saveCmHandleBatch(Collection<YangModelCmHandle> yangModelCmHandles);
+    void saveCmHandleBatch(List<YangModelCmHandle> yangModelCmHandles);
 
     /**
      * Get data node of given cm handle.
index a0aeac3..33d6e9a 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  ============LICENSE_START=======================================================
- *  Copyright (C) 2022-2023 Nordix Foundation
+ *  Copyright (C) 2022-2024 Nordix Foundation
  *  Modifications Copyright (C) 2022 Bell Canada
  *  Modifications Copyright (C) 2023 TechMahindra Ltd.
  *  ================================================================================
 
 package org.onap.cps.ncmp.api.impl.inventory;
 
+import com.google.common.collect.Lists;
 import java.time.OffsetDateTime;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -47,6 +49,8 @@ import org.springframework.stereotype.Component;
 @Component
 public class InventoryPersistenceImpl extends NcmpPersistenceImpl implements InventoryPersistence {
 
+    private static final int CMHANDLE_BATCH_SIZE = 100;
+
     private final CpsModuleService cpsModuleService;
     private final CpsAnchorService cpsAnchorService;
     private final CpsValidator cpsValidator;
@@ -131,19 +135,17 @@ public class InventoryPersistenceImpl extends NcmpPersistenceImpl implements Inv
 
     @Override
     public void saveCmHandle(final YangModelCmHandle yangModelCmHandle) {
-        final String cmHandleJsonData =
-                createCmHandleJsonData(jsonObjectMapper.asJsonString(yangModelCmHandle));
-        cpsDataService.saveListElements(NCMP_DATASPACE_NAME, NCMP_DMI_REGISTRY_ANCHOR, NCMP_DMI_REGISTRY_PARENT,
-                cmHandleJsonData, NO_TIMESTAMP);
+        saveCmHandleBatch(Collections.singletonList(yangModelCmHandle));
     }
 
     @Override
-    public void saveCmHandleBatch(final Collection<YangModelCmHandle> yangModelCmHandles) {
-        final List<String> cmHandlesJsonData = new ArrayList<>();
-        yangModelCmHandles.forEach(yangModelCmHandle -> cmHandlesJsonData.add(
-                createCmHandleJsonData(jsonObjectMapper.asJsonString(yangModelCmHandle))));
-        cpsDataService.saveListElementsBatch(NCMP_DATASPACE_NAME, NCMP_DMI_REGISTRY_ANCHOR,
-                NCMP_DMI_REGISTRY_PARENT, cmHandlesJsonData, NO_TIMESTAMP);
+    public void saveCmHandleBatch(final List<YangModelCmHandle> yangModelCmHandles) {
+        for (final List<YangModelCmHandle> yangModelCmHandleBatch :
+                Lists.partition(yangModelCmHandles, CMHANDLE_BATCH_SIZE)) {
+            final String cmHandlesJsonData = createCmHandlesJsonData(yangModelCmHandleBatch);
+            cpsDataService.saveListElements(NCMP_DATASPACE_NAME, NCMP_DMI_REGISTRY_ANCHOR,
+                    NCMP_DMI_REGISTRY_PARENT, cmHandlesJsonData, NO_TIMESTAMP);
+        }
     }
 
     @Override
@@ -171,7 +173,7 @@ public class InventoryPersistenceImpl extends NcmpPersistenceImpl implements Inv
         return "{\"state\":" + state + "}";
     }
 
-    private static String createCmHandleJsonData(final String yangModelCmHandleAsJson) {
-        return "{\"cm-handles\":[" + yangModelCmHandleAsJson + "]}";
+    private String createCmHandlesJsonData(final List<YangModelCmHandle> yangModelCmHandles) {
+        return "{\"cm-handles\":" + jsonObjectMapper.asJsonString(yangModelCmHandles) + "}";
     }
 }
index 297f18c..cb2f3fd 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  ============LICENSE_START=======================================================
- *  Copyright (C) 2022-2023 Nordix Foundation
+ *  Copyright (C) 2022-2024 Nordix Foundation
  *  Modifications Copyright (C) 2022 Bell Canada
  *  Modifications Copyright (C) 2023 TechMahindra Ltd.
  *  ================================================================================
@@ -227,12 +227,12 @@ class InventoryPersistenceImplSpec extends Specification {
         when: 'the cm handles are saved'
             objectUnderTest.saveCmHandleBatch([yangModelCmHandle1, yangModelCmHandle2])
         then: 'CPS Data Service persists both cm handles as a batch'
-            1 * mockCpsDataService.saveListElementsBatch(NCMP_DATASPACE_NAME, NCMP_DMI_REGISTRY_ANCHOR,
+            1 * mockCpsDataService.saveListElements(NCMP_DATASPACE_NAME, NCMP_DMI_REGISTRY_ANCHOR,
                     NCMP_DMI_REGISTRY_PARENT, _,null) >> {
                 args -> {
-                    def jsonDataList = (args[3] as List)
-                    (jsonDataList[0] as String).contains('cmhandle1')
-                    (jsonDataList[0] as String).contains('cmhandle2')
+                    def jsonData = (args[3] as String)
+                    jsonData.contains('cmhandle1')
+                    jsonData.contains('cmhandle2')
                 }
             }
     }
index 19547bb..1cfe21d 100644 (file)
@@ -97,23 +97,6 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
         addChildrenDataNodes(anchorEntity, parentNodeXpath, newListElements);
     }
 
-    @Override
-    public void addMultipleLists(final String dataspaceName, final String anchorName, final String parentNodeXpath,
-                                 final Collection<Collection<DataNode>> newLists) {
-        final AnchorEntity anchorEntity = getAnchorEntity(dataspaceName, anchorName);
-        final Collection<String> failedXpaths = new HashSet<>();
-        for (final Collection<DataNode> newList : newLists) {
-            try {
-                addChildrenDataNodes(anchorEntity, parentNodeXpath, newList);
-            } catch (final AlreadyDefinedException alreadyDefinedException) {
-                failedXpaths.addAll(alreadyDefinedException.getAlreadyDefinedObjectNames());
-            }
-        }
-        if (!failedXpaths.isEmpty()) {
-            throw AlreadyDefinedException.forDataNodes(failedXpaths, anchorEntity.getName());
-        }
-    }
-
     private void addNewChildDataNode(final AnchorEntity anchorEntity, final String parentNodeXpath,
                                      final DataNode newChild) {
         final FragmentEntity parentFragmentEntity = getFragmentEntity(anchorEntity, parentNodeXpath);
index c987959..0abcc05 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  ============LICENSE_START=======================================================
- *  Copyright (C) 2020-2023 Nordix Foundation
+ *  Copyright (C) 2020-2024 Nordix Foundation
  *  Modifications Copyright (C) 2021 Pantheon.tech
  *  Modifications Copyright (C) 2021-2022 Bell Canada
  *  Modifications Copyright (C) 2022 Deutsche Telekom AG
@@ -99,19 +99,6 @@ public interface CpsDataService {
     void saveListElements(String dataspaceName, String anchorName, String parentNodeXpath, String jsonData,
         OffsetDateTime observedTimestamp);
 
-    /**
-     * Persists child data fragment representing one or more list elements under existing data node for the
-     * given anchor and dataspace.
-     *
-     * @param dataspaceName     dataspace name
-     * @param anchorName        anchor name
-     * @param parentNodeXpath   parent node xpath
-     * @param jsonDataList      collection of json data representing list element(s)
-     * @param observedTimestamp observedTimestamp
-     */
-    void saveListElementsBatch(String dataspaceName, String anchorName, String parentNodeXpath,
-            Collection<String> jsonDataList, OffsetDateTime observedTimestamp);
-
     /**
      * Retrieves all the datanodes by XPath for given dataspace and anchor.
      *
old mode 100755 (executable)
new mode 100644 (file)
index a1bae6a..e49714b
@@ -1,6 +1,6 @@
 /*
  *  ============LICENSE_START=======================================================
- *  Copyright (C) 2021-2023 Nordix Foundation
+ *  Copyright (C) 2021-2024 Nordix Foundation
  *  Modifications Copyright (C) 2020-2022 Bell Canada.
  *  Modifications Copyright (C) 2021 Pantheon.tech
  *  Modifications Copyright (C) 2022-2023 TechMahindra Ltd.
@@ -105,9 +105,9 @@ public class CpsDataServiceImpl implements CpsDataService {
 
     @Override
     @Timed(value = "cps.data.service.list.element.save",
-        description = "Time taken to save a list element")
-    public void saveListElements(final String dataspaceName, final String anchorName,
-        final String parentNodeXpath, final String jsonData, final OffsetDateTime observedTimestamp) {
+        description = "Time taken to save list elements")
+    public void saveListElements(final String dataspaceName, final String anchorName, final String parentNodeXpath,
+                                 final String jsonData, final OffsetDateTime observedTimestamp) {
         cpsValidator.validateNameCharacters(dataspaceName, anchorName);
         final Anchor anchor = cpsAnchorService.getAnchor(dataspaceName, anchorName);
         final Collection<DataNode> listElementDataNodeCollection =
@@ -120,19 +120,6 @@ public class CpsDataServiceImpl implements CpsDataService {
         }
     }
 
-    @Override
-    @Timed(value = "cps.data.service.list.element.batch.save",
-        description = "Time taken to save a batch of list elements")
-    public void saveListElementsBatch(final String dataspaceName, final String anchorName, final String parentNodeXpath,
-            final Collection<String> jsonDataList, final OffsetDateTime observedTimestamp) {
-        cpsValidator.validateNameCharacters(dataspaceName, anchorName);
-        final Anchor anchor = cpsAnchorService.getAnchor(dataspaceName, anchorName);
-        final Collection<Collection<DataNode>> listElementDataNodeCollections =
-                buildDataNodes(anchor, parentNodeXpath, jsonDataList, ContentType.JSON);
-        cpsDataPersistenceService.addMultipleLists(dataspaceName, anchorName, parentNodeXpath,
-                listElementDataNodeCollections);
-    }
-
     @Override
     @Timed(value = "cps.data.service.datanode.get",
             description = "Time taken to get data nodes for an xpath")
@@ -347,14 +334,6 @@ public class CpsDataServiceImpl implements CpsDataService {
         return dataNodes;
     }
 
-    private Collection<Collection<DataNode>> buildDataNodes(final Anchor anchor, final String parentNodeXpath,
-                                                            final Collection<String> nodeDataList,
-                                                            final ContentType contentType) {
-        return nodeDataList.stream()
-            .map(nodeData -> buildDataNodes(anchor, parentNodeXpath, nodeData, contentType))
-            .collect(Collectors.toList());
-    }
-
     private SchemaContext getSchemaContext(final Anchor anchor) {
         return yangTextSchemaSourceSetCache
             .get(anchor.getDataspaceName(), anchor.getSchemaSetName()).getSchemaContext();
index 1baca4e..bc81916 100644 (file)
@@ -1,6 +1,6 @@
 /*
  * ============LICENSE_START=======================================================
- *  Copyright (C) 2020-2023 Nordix Foundation.
+ *  Copyright (C) 2020-2024 Nordix Foundation.
  *  Modifications Copyright (C) 2021 Pantheon.tech
  *  Modifications Copyright (C) 2022 Bell Canada
  *  Modifications Copyright (C) 2022-2023 TechMahindra Ltd.
@@ -66,17 +66,6 @@ public interface CpsDataPersistenceService {
     void addListElements(String dataspaceName, String anchorName, String parentNodeXpath,
         Collection<DataNode> listElementsCollection);
 
-    /**
-     * Add multiple lists of data nodes to a parent node at the same time.
-     *
-     * @param dataspaceName           dataspace name
-     * @param anchorName              anchor name
-     * @param parentNodeXpath         parent node xpath
-     * @param newLists collections of lists of data nodes representing list elements
-     */
-    void addMultipleLists(String dataspaceName, String anchorName, String parentNodeXpath,
-            Collection<Collection<DataNode>> newLists);
-
     /**
      * Retrieves multiple datanodes for a single XPath for given dataspace and anchor.
      * Multiple data nodes are returned when xPath is set to root '/', otherwise single data node
index 77e15c3..322d2c9 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  ============LICENSE_START=======================================================
- *  Copyright (C) 2021-2023 Nordix Foundation
+ *  Copyright (C) 2021-2024 Nordix Foundation
  *  Modifications Copyright (C) 2021 Pantheon.tech
  *  Modifications Copyright (C) 2021-2022 Bell Canada.
  *  Modifications Copyright (C) 2022-2023 TechMahindra Ltd.
@@ -160,24 +160,6 @@ class CpsDataServiceImplSpec extends Specification {
             1 * mockCpsValidator.validateNameCharacters(dataspaceName, anchorName)
     }
 
-    def 'Saving collection of a batch with data fragment under existing node.'() {
-        given: 'schema set for given anchor and dataspace references test-tree model'
-            setupSchemaSetMocks('test-tree.yang')
-        when: 'save data method is invoked with list element json data'
-            def jsonData = '{"branch": [{"name": "A"}, {"name": "B"}]}'
-            objectUnderTest.saveListElementsBatch(dataspaceName, anchorName, '/test-tree', [jsonData], observedTimestamp)
-        then: 'the persistence service method is invoked with correct parameters'
-            1 * mockCpsDataPersistenceService.addMultipleLists(dataspaceName, anchorName, '/test-tree',_) >> {
-                args -> {
-                    def listElementsCollection = args[3] as Collection<Collection<DataNode>>
-                    assert listElementsCollection.size() == 1
-                    def listOfXpaths = listElementsCollection.stream().flatMap(x -> x.stream()).map(it-> it.xpath).collect(Collectors.toList())
-                    assert listOfXpaths.size() == 2
-                    assert listOfXpaths.containsAll(['/test-tree/branch[@name=\'B\']','/test-tree/branch[@name=\'A\']'])
-                }
-            }
-    }
-
     def 'Saving empty list element data fragment.'() {
         given: 'schema set for given anchor and dataspace references test-tree model'
             setupSchemaSetMocks('test-tree.yang')
index b7f4c4f..1d9f806 100644 (file)
@@ -44,6 +44,11 @@ Bug Fixes
 Features
 --------
     - `CPS-2018 <https://jira.onap.org/browse/CPS-2018>`_ Improve performance of CPS update operations.
+    - `CPS-2019 <https://jira.onap.org/browse/CPS-2019>`_ Improve performance of saving CM handles.
+
+Notes
+-----
+    - Java API method CpsDataService::saveListElementsBatch has been removed as part of CPS-2019.
 
 
 Version: 3.4.1
index b107a87..e143099 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  ============LICENSE_START=======================================================
- *  Copyright (C) 2023 Nordix Foundation
+ *  Copyright (C) 2023-2024 Nordix Foundation
  *  Modifications Copyright (C) 2023 TechMahindra Ltd.
  *  ================================================================================
  *  Licensed under the Apache License, Version 2.0 (the 'License');
@@ -276,12 +276,11 @@ class CpsDataServiceIntegrationSpec extends FunctionalSpecBase {
             assert originalCountBookstoreChildNodes == countDataNodesInBookstore()
     }
 
-    def 'Add and Delete a batch of lists (element) data nodes.'() {
-        given: 'two new (categories) data nodes in two separate batches'
-            def json1 = '{"categories": [ {"code":"new1"} ] }'
-            def json2 = '{"categories": [ {"code":"new2"} ] } '
+    def 'Add and Delete a batch of list element data nodes.'() {
+        given: 'two new (categories) data nodes in a single batch'
+            def json = '{"categories": [ {"code":"new1"}, {"code":"new2"} ] }'
         when: 'the batches of new list element(s) are saved'
-            objectUnderTest.saveListElementsBatch(FUNCTIONAL_TEST_DATASPACE_1, BOOKSTORE_ANCHOR_1 , '/bookstore', [json1, json2], now)
+            objectUnderTest.saveListElements(FUNCTIONAL_TEST_DATASPACE_1, BOOKSTORE_ANCHOR_1 , '/bookstore', json, now)
         then: 'they can be retrieved by their xpaths'
             assert objectUnderTest.getDataNodes(FUNCTIONAL_TEST_DATASPACE_1, BOOKSTORE_ANCHOR_1, '/bookstore/categories[@code="new1"]', DIRECT_CHILDREN_ONLY).size() == 1
             assert objectUnderTest.getDataNodes(FUNCTIONAL_TEST_DATASPACE_1, BOOKSTORE_ANCHOR_1, '/bookstore/categories[@code="new2"]', DIRECT_CHILDREN_ONLY).size() == 1
@@ -294,12 +293,11 @@ class CpsDataServiceIntegrationSpec extends FunctionalSpecBase {
             assert originalCountBookstoreChildNodes == countDataNodesInBookstore()
     }
 
-    def 'Add and Delete a batch of lists (element) data nodes with partial success.'() {
-        given: 'two new (categories) data nodes in two separate batches'
-            def jsonNewElement = '{"categories": [ {"code":"new1"} ] }'
-            def jsonExistingElement = '{"categories": [ {"code":"1"} ] } '
+    def 'Add and Delete a batch of list element data nodes with partial success.'() {
+        given: 'one existing and one new (categories) data nodes in a single batch'
+            def json = '{"categories": [ {"code":"new1"}, {"code":"1"} ] }'
         when: 'the batches of new list element(s) are saved'
-            objectUnderTest.saveListElementsBatch(FUNCTIONAL_TEST_DATASPACE_1, BOOKSTORE_ANCHOR_1 , '/bookstore', [jsonNewElement, jsonExistingElement], now)
+            objectUnderTest.saveListElements(FUNCTIONAL_TEST_DATASPACE_1, BOOKSTORE_ANCHOR_1 , '/bookstore', json, now)
         then: 'an already defined (batch) exception is thrown for the existing path'
             def exceptionThrown = thrown(AlreadyDefinedException)
             assert exceptionThrown.alreadyDefinedObjectNames ==  ['/bookstore/categories[@code=\'1\']' ] as Set
index a5669ba..1898f43 100644 (file)
@@ -106,32 +106,4 @@ class WritePerfTest extends CpsPerfTestBase {
             400        || 28               | 250
     }
 
-    def 'Writing openroadm list data using saveListElementsBatch.'() {
-        given: 'an anchor and empty container node for openroadm'
-            cpsAnchorService.createAnchor(CPS_PERFORMANCE_TEST_DATASPACE, LARGE_SCHEMA_SET, WRITE_TEST_ANCHOR)
-            cpsDataService.saveData(CPS_PERFORMANCE_TEST_DATASPACE, WRITE_TEST_ANCHOR,
-                    '{ "openroadm-devices": { "openroadm-device": []}}', now)
-        and: 'a list of device nodes to add'
-            def innerNode = readResourceDataFile('openroadm/innerNode.json')
-            def multipleJsonData = (1..totalNodes).collect {
-                '{ "openroadm-device": [' + innerNode.replace('NODE_ID_HERE', it.toString()) + ']}' }
-        when: 'device nodes are added'
-            resourceMeter.start()
-            cpsDataService.saveListElementsBatch(CPS_PERFORMANCE_TEST_DATASPACE, WRITE_TEST_ANCHOR, '/openroadm-devices', multipleJsonData, OffsetDateTime.now())
-            resourceMeter.stop()
-        then: 'the operation takes less than #expectedDuration and memory used is within limit'
-            recordAndAssertResourceUsage("Saving batch of ${totalNodes} lists",
-                    expectedDuration, resourceMeter.getTotalTimeInSeconds(),
-                    memoryLimit, resourceMeter.getTotalMemoryUsageInMB())
-        cleanup:
-            cpsDataService.deleteDataNodes(CPS_PERFORMANCE_TEST_DATASPACE, WRITE_TEST_ANCHOR, OffsetDateTime.now())
-            cpsAnchorService.deleteAnchor(CPS_PERFORMANCE_TEST_DATASPACE, WRITE_TEST_ANCHOR)
-        where:
-            totalNodes || expectedDuration | memoryLimit
-            50         || 16               | 500
-            100        || 32               | 500
-            200        || 64               | 1000
-            400        || 128              | 1250
-    }
-
 }