phet commented on code in PR #3899:
URL: https://github.com/apache/gobblin/pull/3899#discussion_r1543876278
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManagementTaskStreamImpl.java:
##########
@@ -116,19 +117,20 @@ public boolean hasNext() {
@Override
public DagTask next() {
- try {
LeaseAttemptStatus leaseAttemptStatus = null;
DagActionStore.DagAction dagAction = null;
- while (!(leaseAttemptStatus instanceof
LeaseAttemptStatus.LeaseObtainedStatus)) {
- dagAction = this.dagActionQueue.take(); //`take` blocks till element
is not available
+ while (true) {
+ try {
+ dagAction = this.dagActionQueue.take();
leaseAttemptStatus = retrieveLeaseStatus(dagAction);
Review Comment:
declare these here, rather than earlier setting to `null`
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagManagementTaskStreamImplTest.java:
##########
@@ -88,7 +87,7 @@ public void setUp() throws Exception {
/* This tests adding and removal of dag actions from dag task stream with a
launch task. It verifies that the
{@link DagManagementTaskStreamImpl#next()} call blocks until a {@link
LeaseAttemptStatus.LeaseObtainedStatus} is
returned for a particular action.
- TODO: when we have different dag procs in future, update this test to add
other types of actions (and tasks)
+ TODO: when we have different dag procs in the future, update this test to
add other types of actions (and tasks)
Review Comment:
the alternative would be to make `DMTSImpl::getDagTask`
`@VisibleForTesting`, then to exercise it with each `DagAction`. I prefer how
that streamlines what we're really wishing to validate
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagManagerFlowTest.java:
##########
@@ -58,6 +58,10 @@
import static org.mockito.Mockito.*;
+/**
+ * Tests the state updates (including updating in-memory state and
MysqlDagActionStore) after performing add or cancel
+ * operations by calling addDag, stopDag, kill, and resume. It also tests
flows with and without sla configs.
Review Comment:
this helps!
seems "flow" more in the sense of lifecycle/workflow of dag management,
rather than gaas flows (although the two are related)
--
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]
