phet commented on code in PR #3853:
URL: https://github.com/apache/gobblin/pull/3853#discussion_r1453791241
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -815,13 +806,13 @@ private void pollAndAdvanceDag() throws IOException,
ExecutionException, Interru
break;
}
- if (jobStatus != null && jobStatus.isShouldRetry()) {
+ if (jobStatus.isPresent() && jobStatus.get().isShouldRetry()) {
Review Comment:
more canonical:
```
if (jobStatus.filter(JobStatus::isShouldRetry).isPresent())
```
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -1155,8 +1101,8 @@ private void cleanUp() {
for (Iterator<String> dagIdIterator = this.dagIdstoClean.iterator();
dagIdIterator.hasNext();) {
String dagId = dagIdIterator.next();
Dag<JobExecutionPlan> dag = this.dags.get(dagId);
- JobStatus flowStatus = pollFlowStatus(dag);
- if (flowStatus != null &&
FlowStatusGenerator.FINISHED_STATUSES.contains(flowStatus.getEventName())) {
+ Optional<JobStatus> flowStatus = DagManagerUtils.pollFlowStatus(dag,
this.jobStatusRetriever, this.jobStatusPolledTimer);
+ if (flowStatus.isPresent() &&
FlowStatusGenerator.FINISHED_STATUSES.contains(flowStatus.get().getEventName()))
{
Review Comment:
(suggest equiv. rework here)
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -642,15 +633,15 @@ private void beginResumingDag(DagId dagIdToResume) throws
IOException {
*/
private void finishResumingDags() throws IOException {
for (Map.Entry<String, Dag<JobExecutionPlan>> dag :
this.resumingDags.entrySet()) {
- JobStatus flowStatus = pollFlowStatus(dag.getValue());
- if (flowStatus == null ||
!flowStatus.getEventName().equals(PENDING_RESUME.name())) {
+ Optional<JobStatus> flowStatus =
DagManagerUtils.pollFlowStatus(dag.getValue(), this.jobStatusRetriever,
this.jobStatusPolledTimer);
+ if (!flowStatus.toJavaUtil().filter(fs ->
fs.getEventName().equals(PENDING_RESUME.name())).isPresent()) {
continue;
}
boolean dagReady = true;
for (DagNode<JobExecutionPlan> node : dag.getValue().getNodes()) {
- JobStatus jobStatus = pollJobStatus(node);
- if (jobStatus == null ||
jobStatus.getEventName().equals(FAILED.name()) ||
jobStatus.getEventName().equals(CANCELLED.name())) {
+ Optional<JobStatus> jobStatus = DagManagerUtils.pollJobStatus(node,
this.jobStatusRetriever, this.jobStatusPolledTimer);
+ if (!jobStatus.isPresent() ||
jobStatus.get().getEventName().equals(FAILED.name()) ||
jobStatus.get().getEventName().equals(CANCELLED.name())) {
Review Comment:
separately checking `isPresent()` and then `.get()` over a boolean
short-circuit is not as common as the single expression:
```
if (jobStatus.filter(js -> {
String jobName = js.getEventName();
return jobName.equals(FAILED.name()) || jobName.equals(CANCELLED.name());
}).isPresent())
```
what do you think?
--
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]
