phet commented on code in PR #3721:
URL: https://github.com/apache/gobblin/pull/3721#discussion_r1275529597
##########
gobblin-runtime/src/test/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiterTest.java:
##########
@@ -143,4 +159,128 @@ public void testAcquireLeaseSingleParticipant() throws
Exception {
Assert.assertTrue(sixthObtainedStatus.getEventTimestamp()
<= sixthObtainedStatus.getLeaseAcquisitionTimestamp());
}
+
+ /*
+ Tests CONDITIONALLY_ACQUIRE_LEASE_IF_NEW_ROW_STATEMENT to ensure an
insertion is not attempted unless the table
+ state remains the same as the prior read, which expects no row matching
the primary key in the table
+ Note: this isolates and tests CASE 1 in which another participant could
have acquired the lease between the time
+ the read was done and subsequent write was carried out
+ */
+ @Test //(dependsOnMethods = "testAcquireLeaseSingleParticipant")
+ public void testConditionallyAcquireLeaseIfNewRow() throws IOException {
+ // Inserting the first time should update 1 row
+ int numRowsUpdated =
this.mysqlMultiActiveLeaseArbiter.withPreparedStatement(formattedAcquireLeaseNewRowStatement,
+ insertStatement -> {
+ completeInsertPreparedStatement(insertStatement, resumeDagAction);
+ return insertStatement.executeUpdate();
+ }, true);
+ Assert.assertEquals(numRowsUpdated, 1);
Review Comment:
overall, the `tryAcquireLease` impl deserves more abstraction. it's
presently 110 LoC... with SO MANY details spread throughout.
naturally, one could test either the SQL or the logic flow, but from what
you describe, it sounds like your aim is to test the logic. for instance, if
you created abstractions to distill the first 40 lines of `tryAcquireLease` to
this:
```
// Check table for an existing entry for this flow action and event time
Optional<GetEventInfoResult> getResult =
getExistingEventInfo(flowAction);
try {
if (!getResult.isPresent()) {
log.debug("tryAcquireLease for [{}, eventTimestamp: {}] - CASE 1: no
existing row for this flow action, then go"
+ " ahead and insert", flowAction, eventTimeMillis);
boolean didAcquireLease = initializeToAcquireLease(flowAction);
return evaluateStatusAfterLeaseAttempt(didAcquireLease, flowAction,
Optional.absent());
}
// [continues...]
```
couldn't you merely test `evaluateStatusAfterLeaseAttempt` to see what it
does (perhaps via mocking) when the first param is `false`?
meanwhile, when it comes to testing SQL itself, use the systematic use of
`withPreparedStatement` to your advantage. i.e. exercise various methods and
intercept `withPreparedStatement` to validate that the expected SQL is passed
as the first arg.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
