khandelwal-prateek commented on code in PR #4084:
URL: https://github.com/apache/gobblin/pull/4084#discussion_r1882538576
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagProcessingEngine.java:
##########
@@ -149,6 +158,13 @@ public void run() {
dagTask.conclude();
log.info(dagProc.contextualizeStatus("concluded dagTask"));
} catch (Exception e) {
+ if(!DagProcessingEngine.isTransientException(e)) {
+ log.error("Ignoring non transient exception. DagTask {} will
conclude and will not be retried. Exception - {} ",
+ dagTask, e);
Review Comment:
the exception is included as part of the message string instead of passed as
a separate parameter, this would log only the exception's toString(), without
the full stack trace.
Please update the log statement to pass the exception as a separate
parameter to log the stack trace
```
log.error("Ignoring non-transient exception. DagTask {} will conclude and
will not be retried.", dagTask, e);
```
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagProcessingEngine.java:
##########
@@ -149,6 +158,13 @@ public void run() {
dagTask.conclude();
log.info(dagProc.contextualizeStatus("concluded dagTask"));
} catch (Exception e) {
+ if(!DagProcessingEngine.isTransientException(e)) {
+ log.error("Ignoring non transient exception. DagTask {} will
conclude and will not be retried. Exception - {} ",
Review Comment:
don't see `toString()` for dagTask, what does dagTask log? It would be
useful to have dagId & dagAction in the log.
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagProcessingEngine.java:
##########
@@ -67,6 +70,8 @@ public class DagProcessingEngine extends AbstractIdleService {
public static final String DEFAULT_JOB_START_DEADLINE_TIME_MS =
"defaultJobStartDeadlineTimeMillis";
@Getter static long defaultJobStartDeadlineTimeMillis;
public static final String DEFAULT_FLOW_FAILURE_OPTION =
FailureOption.FINISH_ALL_POSSIBLE.name();
+ // Todo Update to fetch list from config once transient exception handling
is implemented and retryable exceptions defined
Review Comment:
nit: the convention is to use `TODO: `, please update from `Todo` to `TODO:
` for consistency
--
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-unsubscr...@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
