[GitHub] [gobblin] phet commented on a diff in pull request #3715: [GOBBLIN-1851] Unit tests for MysqlMultiActiveLeaseArbiter with Single Participant

["phet (via GitHub)" <gi...@apache.org>]

phet commented on code in PR #3715:
URL: https://github.com/apache/gobblin/pull/3715#discussion_r1265977660


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiter.java:
##########
@@ -287,32 +304,43 @@ else if (leaseValidityStatus == 2) {
     }
   }
 
-  protected Optional<GetEventInfoResult> createGetInfoResult(ResultSet resultSet) {
+  protected GetEventInfoResult createGetInfoResult(ResultSet resultSet) throws SQLException {
     try {
       // Extract values from result set
       Timestamp dbEventTimestamp = resultSet.getTimestamp("event_timestamp");
       Timestamp dbLeaseAcquisitionTimestamp = resultSet.getTimestamp("lease_acquisition_timestamp");
-      boolean withinEpsilon = resultSet.getBoolean("isWithinEpsilon");
-      int leaseValidityStatus = resultSet.getInt("leaseValidityStatus");
+      boolean withinEpsilon = resultSet.getBoolean("is_within_epsilon");
+      int leaseValidityStatus = resultSet.getInt("lease_validity_status");
       int dbLinger = resultSet.getInt("linger");
       Timestamp dbCurrentTimestamp = resultSet.getTimestamp("CURRENT_TIMESTAMP");
-      return Optional.of(new GetEventInfoResult(dbEventTimestamp, dbLeaseAcquisitionTimestamp, withinEpsilon, leaseValidityStatus,
-          dbLinger, dbCurrentTimestamp));
-    } catch (SQLException exception) {
-      log.warn("Failed to retrieve values from GET event info query resultSet. Exception: ", exception);
-      // Note: this will proceed to CASE 1 of acquiring a lease above
-      return Optional.absent();
+      return new GetEventInfoResult(dbEventTimestamp, dbLeaseAcquisitionTimestamp, withinEpsilon, leaseValidityStatus,
+          dbLinger, dbCurrentTimestamp);
+    } catch (SQLException e) {
+      throw e;
+    } finally {
+      if (resultSet != null) {
+        resultSet.close();
+      }
     }
   }
 
   protected SelectInfoResult createSelectInfoResult(ResultSet resultSet) throws SQLException {
-      if (!resultSet.next()) {
-        log.error("Expected num rows and lease_acquisition_timestamp returned from query but received nothing");
+      try {
+        if (!resultSet.next()) {
+          resultSet.close();

Review Comment:
   this is already in the `finally`... why add here too? (seems confusing)



##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiter.java:
##########
@@ -287,32 +304,43 @@ else if (leaseValidityStatus == 2) {
     }
   }
 
-  protected Optional<GetEventInfoResult> createGetInfoResult(ResultSet resultSet) {
+  protected GetEventInfoResult createGetInfoResult(ResultSet resultSet) throws SQLException {
     try {
       // Extract values from result set
       Timestamp dbEventTimestamp = resultSet.getTimestamp("event_timestamp");
       Timestamp dbLeaseAcquisitionTimestamp = resultSet.getTimestamp("lease_acquisition_timestamp");
-      boolean withinEpsilon = resultSet.getBoolean("isWithinEpsilon");
-      int leaseValidityStatus = resultSet.getInt("leaseValidityStatus");
+      boolean withinEpsilon = resultSet.getBoolean("is_within_epsilon");
+      int leaseValidityStatus = resultSet.getInt("lease_validity_status");
       int dbLinger = resultSet.getInt("linger");
       Timestamp dbCurrentTimestamp = resultSet.getTimestamp("CURRENT_TIMESTAMP");
-      return Optional.of(new GetEventInfoResult(dbEventTimestamp, dbLeaseAcquisitionTimestamp, withinEpsilon, leaseValidityStatus,
-          dbLinger, dbCurrentTimestamp));
-    } catch (SQLException exception) {
-      log.warn("Failed to retrieve values from GET event info query resultSet. Exception: ", exception);
-      // Note: this will proceed to CASE 1 of acquiring a lease above
-      return Optional.absent();
+      return new GetEventInfoResult(dbEventTimestamp, dbLeaseAcquisitionTimestamp, withinEpsilon, leaseValidityStatus,
+          dbLinger, dbCurrentTimestamp);
+    } catch (SQLException e) {
+      throw e;

Review Comment:
   1. if this is all you want, it's the implicit behavior, which you need not write explicitly.
   
   2. even so, elsewhere, we wrap `SQLException` in an `IOException`.  do we want that here too... or is there already a higher layer wrapping around this invocation where that will happen for us?



##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiter.java:
##########
@@ -375,17 +473,61 @@ public boolean recordLeaseSuccess(LeaseObtainedStatus status)
   }
 
   /** Abstracts recurring pattern around resource management and exception re-mapping. */
-  protected <T> T withPreparedStatement(String sql, CheckedFunction<PreparedStatement, T> f, boolean shouldCommit) throws IOException {
+  protected <T> T withPreparedStatement(String sql, CheckedFunction<PreparedStatement, T> f, boolean shouldCommit)
+      throws IOException {
     try (Connection connection = this.dataSource.getConnection();
         PreparedStatement statement = connection.prepareStatement(sql)) {
       T result = f.apply(statement);
       if (shouldCommit) {
         connection.commit();
       }
+      statement.close();
       return result;
     } catch (SQLException e) {
-      log.warn("Received SQL exception that can result from invalid connection. Checking if validation query is set {} Exception is {}", ((HikariDataSource) this.dataSource).getConnectionTestQuery(), e);
+      log.warn("Received SQL exception that can result from invalid connection. Checking if validation query is set {} "
+          + "Exception is {}", ((HikariDataSource) this.dataSource).getConnectionTestQuery(), e);
       throw new IOException(e);
     }
   }
+
+
+  /**
+   * DTO for arbiter's current lease state for a FlowActionEvent
+  */
+  @Data
+  static class GetEventInfoResult {
+    private Timestamp dbEventTimestamp;
+    private Timestamp dbLeaseAcquisitionTimestamp;
+    private boolean withinEpsilon;
+    private int leaseValidityStatus;
+    private int dbLinger;
+    private Timestamp dbCurrentTimestamp;
+
+    GetEventInfoResult(Timestamp eventTimestamp, Timestamp leaseAcquisitionTimestamp, boolean isWithinEpsilon,
+        int validityStatus, int linger, Timestamp currentTimestamp) {
+      // Extract values from result set
+      dbEventTimestamp = eventTimestamp;
+      dbLeaseAcquisitionTimestamp = leaseAcquisitionTimestamp;
+      withinEpsilon = isWithinEpsilon;
+      leaseValidityStatus = validityStatus;
+      dbLinger = linger;
+      dbCurrentTimestamp = currentTimestamp;
+    }

Review Comment:
   won't the `@Data` annotation generate such an all-args ctor for us?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org