dmvk commented on code in PR #22506:
URL: https://github.com/apache/flink/pull/22506#discussion_r1182229885
##########
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/ErrorInfo.java:
##########
@@ -74,6 +83,11 @@ public ErrorInfo(@Nonnull Throwable exception, long
timestamp) {
? (SerializedThrowable) exception
: new SerializedThrowable(exception);
this.timestamp = timestamp;
+ this.labels = labels;
Review Comment:
nit
```suggestion
this.labels = Preconditions.checkNotNull(labels);
```
##########
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/exceptionhistory/RootExceptionHistoryEntry.java:
##########
@@ -107,18 +113,23 @@ public static RootExceptionHistoryEntry
fromExceptionHistoryEntry(
public static RootExceptionHistoryEntry fromGlobalFailure(ErrorInfo
errorInfo) {
Preconditions.checkNotNull(errorInfo, "errorInfo");
return fromGlobalFailure(
- errorInfo.getException(), errorInfo.getTimestamp(),
Collections.emptyList());
+ errorInfo.getException(),
+ errorInfo.getTimestamp(),
+ errorInfo.getLabels(),
+ Collections.emptyList());
}
private static RootExceptionHistoryEntry createRootExceptionHistoryEntry(
Throwable cause,
long timestamp,
+ Map<String, String> labels,
Review Comment:
the use of nullable seems inconsistent
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1132,6 +1159,16 @@ private void runRequestNextInputSplitTest(
expectedRemainingInputSplits
.apply(inputSplitsPerTask)
.toArray(EMPTY_TESTING_INPUT_SPLITS));
+
+ // Make sure FailureEnrichers are triggered for the above failure
+ assertThat(testingEnricher.seenThrowable.stream().map(t ->
t.getMessage()))
Review Comment:
Why is the failure enrichment tested in `testRequestNextInputSplit`? We
should have a dedicated test for new features/regressions.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/ErrorInfo.java:
##########
@@ -66,6 +70,11 @@ public static Throwable handleMissingThrowable(@Nullable
Throwable throwable) {
}
public ErrorInfo(@Nonnull Throwable exception, long timestamp) {
+ this(exception, timestamp, null);
+ }
+
+ public ErrorInfo(
+ @Nonnull Throwable exception, long timestamp, @Nullable
Map<String, String> labels) {
Review Comment:
The use of nullable seems to be inconsistent in this class. Would it make
sense to use an empty map instead of nulls?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/exceptionhistory/ExceptionHistoryEntryMatcher.java:
##########
@@ -87,6 +107,18 @@ protected boolean matchesSafely(
match = false;
}
+ if (exceptionHistoryEntry.getLabels() == null) {
+ if (expectedLabels != null) {
+ description.appendText(" actualLabels=null");
+ match = false;
+ }
+ } else if (exceptionHistoryEntry.getLabels().equals(expectedLabels)) {
+ description
+ .appendText(" actualLabels=")
+
.appendText(String.valueOf(exceptionHistoryEntry.getLabels()));
+ match = false;
Review Comment:
I'm not able to wrap my head around this. How come we don't have a match if
the actual labels are equal to what is expected?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/utils/JobMasterBuilder.java:
##########
@@ -139,6 +144,11 @@ public JobMasterBuilder
withFatalErrorHandler(FatalErrorHandler fatalErrorHandle
return this;
}
+ public JobMasterBuilder withFailureEnriches(Collection<FailureEnricher>
failureEnrichers) {
Review Comment:
```suggestion
public JobMasterBuilder withFailureEnrichers(Collection<FailureEnricher>
failureEnrichers) {
```
##########
flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/factories/DefaultJobMasterServiceFactory.java:
##########
@@ -122,6 +123,7 @@ private JobMasterService internalCreateJobMasterService(
DefaultExecutionDeploymentReconciler::new,
BlocklistUtils.loadBlocklistHandlerFactory(
jobMasterConfiguration.getConfiguration()),
+ Collections.emptySet(),
Review Comment:
Are we missing something here?
--
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]
