Use try-with-resources to close BatchHandler (#15661) (#15704)

* Use try-with-resources to close BatchHandler * Rework test to deal with multi-threading * Add try-catch to Analytics table manager
dhis2 · Nov 15, 2023 · 9a55cc6 · 9a55cc6
1 parent ed1dc2f
commit 9a55cc6
Show file tree

Hide file tree

Showing 7 changed files with 61 additions and 53 deletions.
diff --git a/...ytics/src/main/java/org/hisp/dhis/analytics/table/JdbcOwnershipAnalyticsTableManager.java b/...ytics/src/main/java/org/hisp/dhis/analytics/table/JdbcOwnershipAnalyticsTableManager.java
@@ -176,30 +176,32 @@ private void populateTableInternal(AnalyticsTablePartition partition, String sql
     List<String> columnNames =
         getDimensionColumns().stream().map(AnalyticsTableColumn::getName).collect(toList());
 
-    MappingBatchHandler batchHandler =
+    try (MappingBatchHandler batchHandler =
         MappingBatchHandler.builder()
             .jdbcConfiguration(jdbcConfiguration)
             .tableName(partition.getTempTableName())
             .columns(columnNames)
-            .build();
-
-    batchHandler.init();
-
-    JdbcOwnershipWriter writer = JdbcOwnershipWriter.getInstance(batchHandler);
-    AtomicInteger queryRowCount = new AtomicInteger();
-
-    jdbcTemplate.query(
-        sql,
-        resultSet -> {
-          writer.write(getRowMap(columnNames, resultSet));
-          queryRowCount.getAndIncrement();
-        });
-
-    log.info(
-        "OwnershipAnalytics query row count was {} for {}",
-        queryRowCount,
-        partition.getTempTableName());
-    batchHandler.flush();
+            .build()) {
+      batchHandler.init();
+
+      JdbcOwnershipWriter writer = JdbcOwnershipWriter.getInstance(batchHandler);
+      AtomicInteger queryRowCount = new AtomicInteger();
+
+      jdbcTemplate.query(
+          sql,
+          resultSet -> {
+            writer.write(getRowMap(columnNames, resultSet));
+            queryRowCount.getAndIncrement();
+          });
+
+      log.info(
+          "OwnershipAnalytics query row count was {} for {}",
+          queryRowCount,
+          partition.getTempTableName());
+      batchHandler.flush();
+    } catch (Exception ex) {
+      log.error("Failed to alter table ownership: ", ex);
+    }
   }
 
   private String getInputSql(Program program) {

diff --git a/...in/java/org/hisp/dhis/dxf2/dataset/DefaultCompleteDataSetRegistrationExchangeService.java b/...in/java/org/hisp/dhis/dxf2/dataset/DefaultCompleteDataSetRegistrationExchangeService.java
@@ -244,9 +244,9 @@ public ImportSummary saveCompleteDataSetRegistrationsJson(
   private ImportSummary saveCompleteDataSetRegistrations(
       ImportOptions importOptions,
       Callable<CompleteDataSetRegistrations> deserializeRegistrations) {
-    BatchHandler<CompleteDataSetRegistration> batchHandler =
-        batchHandlerFactory.createBatchHandler(CompleteDataSetRegistrationBatchHandler.class);
-    try {
+
+    try (BatchHandler<CompleteDataSetRegistration> batchHandler =
+        batchHandlerFactory.createBatchHandler(CompleteDataSetRegistrationBatchHandler.class)) {
       CompleteDataSetRegistrations completeDataSetRegistrations = deserializeRegistrations.call();
       ImportSummary summary =
           saveCompleteDataSetRegistrations(
@@ -256,7 +256,7 @@ private ImportSummary saveCompleteDataSetRegistrations(
 
       return summary;
     } catch (Exception ex) {
-      batchHandler.flush();
+      log.error("Complete data set registrations could not be saved.");
       return handleImportError(ex);
     }
   }

diff --git a/...ervice-dxf2/src/main/java/org/hisp/dhis/dxf2/datavalueset/DefaultDataValueSetService.java b/...ervice-dxf2/src/main/java/org/hisp/dhis/dxf2/datavalueset/DefaultDataValueSetService.java
@@ -623,14 +623,13 @@ private ImportSummary importDataValueSet(
       ImportOptions options, JobConfiguration id, Callable<DataValueSetReader> createReader) {
     options = ObjectUtils.firstNonNull(options, ImportOptions.getDefaultImportOptions());
 
-    BatchHandler<DataValue> dvBatch =
-        batchHandlerFactory.createBatchHandler(DataValueBatchHandler.class);
-    BatchHandler<DataValueAudit> dvaBatch =
-        batchHandlerFactory.createBatchHandler(DataValueAuditBatchHandler.class);
-
     notifier.clear(id);
 
-    try (DataValueSetReader reader = createReader.call()) {
+    try (BatchHandler<DataValue> dvBatch =
+            batchHandlerFactory.createBatchHandler(DataValueBatchHandler.class);
+        BatchHandler<DataValueAudit> dvaBatch =
+            batchHandlerFactory.createBatchHandler(DataValueAuditBatchHandler.class);
+        DataValueSetReader reader = createReader.call()) {
       ImportSummary summary = importDataValueSet(options, id, reader, dvBatch, dvaBatch);
 
       dvBatch.flush();
@@ -643,8 +642,6 @@ private ImportSummary importDataValueSet(
 
       return summary;
     } catch (Exception ex) {
-      dvBatch.flush();
-      dvaBatch.flush();
       log.error(DebugUtils.getStackTrace(ex));
       notifier.notify(id, ERROR, "Process failed: " + ex.getMessage(), true);
       return new ImportSummary(ImportStatus.ERROR, "The import process failed: " + ex.getMessage());

diff --git a/...est/java/org/hisp/dhis/jdbc/batchhandler/CompleteDataSetRegistrationBatchHandlerTest.java b/...est/java/org/hisp/dhis/jdbc/batchhandler/CompleteDataSetRegistrationBatchHandlerTest.java
@@ -92,11 +92,11 @@ class CompleteDataSetRegistrationBatchHandlerTest extends IntegrationTestBase {
 
   private CompleteDataSetRegistration regD;
 
-  private Date now = new Date();
+  private final Date now = new Date();
 
-  private String storedBy = "johndoe";
+  private final String storedBy = "johndoe";
 
-  private String lastUpdatedBy = "johndoe";
+  private final String lastUpdatedBy = "johndoe";
 
   // -------------------------------------------------------------------------
   // Fixture
@@ -165,9 +165,7 @@ public void setUpTest() {
   }
 
   @Override
-  public void tearDownTest() {
-    batchHandler.flush();
-  }
+  public void tearDownTest() {}
 
   // -------------------------------------------------------------------------
   // Tests

diff --git a/...gration/src/test/java/org/hisp/dhis/jdbc/batchhandler/DataValueAuditBatchHandlerTest.java b/...gration/src/test/java/org/hisp/dhis/jdbc/batchhandler/DataValueAuditBatchHandlerTest.java
@@ -72,8 +72,6 @@ class DataValueAuditBatchHandlerTest extends SingleSetupIntegrationTestBase {
 
   @Autowired private CategoryService categoryService;
 
-  private BatchHandler<DataValueAudit> batchHandler;
-
   private DataElement dataElementA;
 
   private CategoryOptionCombo categoryOptionComboA;
@@ -107,7 +105,7 @@ class DataValueAuditBatchHandlerTest extends SingleSetupIntegrationTestBase {
   // -------------------------------------------------------------------------
   @Override
   public void setUpTest() {
-    batchHandler = batchHandlerFactory.createBatchHandler(DataValueAuditBatchHandler.class);
+
     dataElementA = createDataElement('A');
     dataElementService.addDataElement(dataElementA);
     categoryOptionComboA = categoryService.getDefaultCategoryOptionCombo();
@@ -132,19 +130,18 @@ public void setUpTest() {
     auditB = new DataValueAudit(dataValueA, "12", storedBy, AuditType.UPDATE);
     auditC = new DataValueAudit(dataValueB, "21", storedBy, AuditType.UPDATE);
     auditD = new DataValueAudit(dataValueB, "22", storedBy, AuditType.UPDATE);
-    batchHandler.init();
   }
 
   @Override
-  public void tearDownTest() {
-    batchHandler.flush();
-  }
+  public void tearDownTest() {}
 
   // -------------------------------------------------------------------------
   // Tests
   // -------------------------------------------------------------------------
   @Test
   void testAddObject() {
+    BatchHandler<DataValueAudit> batchHandler;
+    batchHandler = batchHandlerFactory.createBatchHandler(DataValueAuditBatchHandler.class).init();
     batchHandler.addObject(auditA);
     batchHandler.addObject(auditB);
     batchHandler.addObject(auditC);
@@ -161,6 +158,8 @@ void testAddObject() {
   /** DataValueAudit can never equal another. */
   @Test
   void testObjectExists() {
+    BatchHandler<DataValueAudit> batchHandler;
+    batchHandler = batchHandlerFactory.createBatchHandler(DataValueAuditBatchHandler.class).init();
     auditService.addDataValueAudit(auditA);
     auditService.addDataValueAudit(auditB);
     assertFalse(batchHandler.objectExists(auditA));
@@ -171,6 +170,8 @@ void testObjectExists() {
 
   @Test
   void testUpdateObject() {
+    BatchHandler<DataValueAudit> batchHandler;
+    batchHandler = batchHandlerFactory.createBatchHandler(DataValueAuditBatchHandler.class).init();
     auditService.addDataValueAudit(auditA);
     auditA.setModifiedBy("bill");
     batchHandler.updateObject(auditA);

diff --git a/...-integration/src/test/java/org/hisp/dhis/jdbc/batchhandler/DataValueBatchHandlerTest.java b/...-integration/src/test/java/org/hisp/dhis/jdbc/batchhandler/DataValueBatchHandlerTest.java
@@ -72,8 +72,6 @@ class DataValueBatchHandlerTest extends IntegrationTestBase {
 
   @Autowired private CategoryService categoryService;
 
-  private BatchHandler<DataValue> batchHandler;
-
   private DataElement dataElementA;
 
   private CategoryOptionCombo categoryOptionComboA;
@@ -105,7 +103,7 @@ class DataValueBatchHandlerTest extends IntegrationTestBase {
   // -------------------------------------------------------------------------
   @Override
   public void setUpTest() {
-    batchHandler = batchHandlerFactory.createBatchHandler(DataValueBatchHandler.class);
+
     dataElementA = createDataElement('A');
     dataElementService.addDataElement(dataElementA);
     categoryOptionComboA = categoryService.getDefaultCategoryOptionCombo();
@@ -142,19 +140,19 @@ public void setUpTest() {
             dataElementA, periodB, unitB, categoryOptionComboA, categoryOptionComboA, "15");
     // with
     // 4th
-    batchHandler.init();
+
   }
 
   @Override
-  public void tearDownTest() {
-    batchHandler.flush();
-  }
+  public void tearDownTest() {}
 
   // -------------------------------------------------------------------------
   // Tests
   // -------------------------------------------------------------------------
   @Test
   void testInsertObject() {
+    BatchHandler<DataValue> batchHandler;
+    batchHandler = batchHandlerFactory.createBatchHandler(DataValueBatchHandler.class).init();
     batchHandler.insertObject(dataValueA);
     DataValue dataValue =
         dataValueService.getDataValue(
@@ -164,6 +162,8 @@ void testInsertObject() {
 
   @Test
   void testAddObject() {
+    BatchHandler<DataValue> batchHandler;
+    batchHandler = batchHandlerFactory.createBatchHandler(DataValueBatchHandler.class).init();
     batchHandler.addObject(dataValueA);
     batchHandler.addObject(dataValueB);
     batchHandler.addObject(dataValueC);
@@ -185,6 +185,8 @@ void testAddObject() {
 
   @Test
   void testAddObjectDuplicates() {
+    BatchHandler<DataValue> batchHandler;
+    batchHandler = batchHandlerFactory.createBatchHandler(DataValueBatchHandler.class).init();
     batchHandler.addObject(dataValueA);
     batchHandler.addObject(dataValueB);
     batchHandler.addObject(dataValueC);
@@ -208,6 +210,8 @@ void testAddObjectDuplicates() {
 
   @Test
   void testFindObject() {
+    BatchHandler<DataValue> batchHandler;
+    batchHandler = batchHandlerFactory.createBatchHandler(DataValueBatchHandler.class).init();
     dataValueService.addDataValue(dataValueA);
     dataValueService.addDataValue(dataValueC);
     DataValue retrievedDataValueA = batchHandler.findObject(dataValueA);
@@ -224,6 +228,8 @@ void testFindObject() {
 
   @Test
   void testObjectExists() {
+    BatchHandler<DataValue> batchHandler;
+    batchHandler = batchHandlerFactory.createBatchHandler(DataValueBatchHandler.class).init();
     dataValueService.addDataValue(dataValueA);
     dataValueService.addDataValue(dataValueC);
     assertTrue(batchHandler.objectExists(dataValueA));
@@ -235,6 +241,8 @@ void testObjectExists() {
   @Test
   @Disabled("ERROR: cannot execute UPDATE in a read-only transaction")
   void testUpdateObject() {
+    BatchHandler<DataValue> batchHandler;
+    batchHandler = batchHandlerFactory.createBatchHandler(DataValueBatchHandler.class).init();
     dataValueService.addDataValue(dataValueA);
     dataValueA.setValue("20");
     batchHandler.updateObject(dataValueA);
@@ -247,6 +255,8 @@ void testUpdateObject() {
 
   @Test
   void testDeleteObject() {
+    BatchHandler<DataValue> batchHandler;
+    batchHandler = batchHandlerFactory.createBatchHandler(DataValueBatchHandler.class).init();
     dataValueService.addDataValue(dataValueA);
     dataValueService.addDataValue(dataValueC);
     assertTrue(batchHandler.objectExists(dataValueA));

diff --git a/dhis-2/pom.xml b/dhis-2/pom.xml
@@ -89,7 +89,7 @@
     <dhis2-rule-engine.version>2.1.8.0</dhis2-rule-engine.version>
 
     <!-- HISP Quick and Staxwax -->
-    <dhis-hisp-quick.version>1.4.1</dhis-hisp-quick.version>
+    <dhis-hisp-quick.version>1.4.3</dhis-hisp-quick.version>
     <dhis-hisp-staxwax.version>2.0.0</dhis-hisp-staxwax.version>
     <dhis-json-tree.version>0.5.0</dhis-json-tree.version>