[
https://issues.apache.org/jira/browse/GOBBLIN-2073?focusedWorklogId=921349&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-921349
]
ASF GitHub Bot logged work on GOBBLIN-2073:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 30/May/24 08:34
Start Date: 30/May/24 08:34
Worklog Time Spent: 10m
Work Description: phet commented on code in PR #3952:
URL: https://github.com/apache/gobblin/pull/3952#discussion_r1620142774
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/MysqlMultiActiveLeaseArbiterTest.java:
##########
@@ -264,12 +269,19 @@ public void
testConditionallyAcquireLeaseIfFinishedLeasingStatement()
*/
@Test (dependsOnMethods =
"testConditionallyAcquireLeaseIfFinishedLeasingStatement")
public void testOlderReminderEventAcquireLease() throws IOException {
+ // Use current time as flowName to create unique event
+ DagActionStore.DagAction newLaunchAction = new
DagActionStore.DagAction(flowGroup,
+ String.valueOf(System.currentTimeMillis()), flowExecutionId, jobName,
DagActionStore.DagActionType.LAUNCH);
Review Comment:
please avoid "current time" in a test. instead use a known value.
unless this is somehow to be called in a loop, couldn't we just give it a
static yet distinct flow name.
confusing name too, given there's already a `launchDagAction2`.
also, are tests like this the only callers of the 5-param version of the DA
ctor?
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/MysqlMultiActiveLeaseArbiterTest.java:
##########
@@ -264,12 +269,19 @@ public void
testConditionallyAcquireLeaseIfFinishedLeasingStatement()
*/
@Test (dependsOnMethods =
"testConditionallyAcquireLeaseIfFinishedLeasingStatement")
public void testOlderReminderEventAcquireLease() throws IOException {
+ // Use current time as flowName to create unique event
+ DagActionStore.DagAction newLaunchAction = new
DagActionStore.DagAction(flowGroup,
+ String.valueOf(System.currentTimeMillis()), flowExecutionId, jobName,
DagActionStore.DagActionType.LAUNCH);
Review Comment:
please avoid "current time" in a test. instead use a known value.
unless this is somehow to be called in a loop, couldn't we just give it a
static yet distinct flow name?
confusing name too, given there's already a `launchDagAction2`.
also, are tests like this the only callers of the 5-param version of the DA
ctor?
Issue Time Tracking
-------------------
Worklog Id: (was: 921349)
Time Spent: 40m (was: 0.5h)
> Add eventTime and reminder fields to DagAction POJO
> ---------------------------------------------------
>
> Key: GOBBLIN-2073
> URL: https://issues.apache.org/jira/browse/GOBBLIN-2073
> Project: Apache Gobblin
> Issue Type: Bug
> Components: gobblin-service
> Reporter: Urmi Mustafi
> Assignee: Abhishek Tiwari
> Priority: Major
> Time Spent: 40m
> Remaining Estimate: 0h
>
> An event timestamp and reminder flag are used in conjunction with DagActions
> for lease arbitration and for reminder dagActions. It makes sense to include
> these fields in the DagAction object itself to easily transfer the
> information from one location to another. In particular for reminder events
> used to check back in on incomplete dagAction leases, these two fields are
> utilized and it's easiest to keep track of them when it's maintained within
> the consensusDagAction returned in a LeaseAttemptStatus
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
