[
https://issues.apache.org/jira/browse/GOBBLIN-1859?focusedWorklogId=873182&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-873182
]
ASF GitHub Bot logged work on GOBBLIN-1859:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 26/Jul/23 22:11
Start Date: 26/Jul/23 22:11
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 873182)
Time Spent: 1h 40m (was: 1.5h)
> Multi-active Unit Test for Multiple Participant
> -----------------------------------------------
>
> Key: GOBBLIN-1859
> URL: https://issues.apache.org/jira/browse/GOBBLIN-1859
> Project: Apache Gobblin
> Issue Type: Bug
> Components: gobblin-service
> Reporter: Urmi Mustafi
> Assignee: Abhishek Tiwari
> Priority: Major
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> In multi-active mode, multiple participants will be reading/writing to MySQL
> table. We want to make sure each participant acts on the table in a manner
> that takes into account that the state may have changed while the
> MultiActiveLeaseArbiter is processing the result of a READ. This PR adds unit
> tests that validate SQL Insertion statements are conditional upon the state
> of the particular row corresponding to the flow action event being unchanged
> from the read made by this participant.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
