huwh commented on code in PR #23386:
URL: https://github.com/apache/flink/pull/23386#discussion_r1323944084
##########
flink-runtime/src/test/java/org/apache/flink/runtime/failure/FailureEnricherUtilsTest.java:
##########
@@ -109,6 +109,15 @@ public void testGetFailureEnrichers() {
assertThat(enrichers).hasSize(1);
// verify that the failure enricher was created and returned
assertThat(enrichers.iterator().next()).isInstanceOf(TestEnricher.class);
+
+ // Valid plus Invalid Name combination
+ configuration.set(
+ JobManagerOptions.FAILURE_ENRICHERS_LIST,
+ FailureEnricherUtilsTest.class.getName() + "," +
TestEnricher.class.getName());
+ final Collection<FailureEnricher> validInvalidEnrichers =
+ FailureEnricherUtils.getFailureEnrichers(configuration,
createPluginManager());
+ assertThat(validInvalidEnrichers).hasSize(1);
+
assertThat(enrichers.iterator().next()).isInstanceOf(TestEnricher.class);
Review Comment:
It's appreciated if you could update line 109~111.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/failure/FailureEnricherUtilsTest.java:
##########
@@ -109,6 +109,15 @@ public void testGetFailureEnrichers() {
assertThat(enrichers).hasSize(1);
// verify that the failure enricher was created and returned
assertThat(enrichers.iterator().next()).isInstanceOf(TestEnricher.class);
+
+ // Valid plus Invalid Name combination
+ configuration.set(
+ JobManagerOptions.FAILURE_ENRICHERS_LIST,
+ FailureEnricherUtilsTest.class.getName() + "," +
TestEnricher.class.getName());
+ final Collection<FailureEnricher> validInvalidEnrichers =
+ FailureEnricherUtils.getFailureEnrichers(configuration,
createPluginManager());
+ assertThat(validInvalidEnrichers).hasSize(1);
+
assertThat(enrichers.iterator().next()).isInstanceOf(TestEnricher.class);
Review Comment:
It is recommended to verify this way to make it clearer.
```suggestion
assertThat(validInvalidEnrichers)
.satisfiesExactly(
enricher ->
assertThat(enricher).isInstanceOf(TestEnricher.class));
```
##########
flink-runtime/src/main/java/org/apache/flink/runtime/failure/FailureEnricherUtils.java:
##########
@@ -105,6 +105,16 @@ static Collection<FailureEnricher> getFailureEnrichers(
includedEnrichers);
}
}
+ includedEnrichers.removeAll(
Review Comment:
We could remove the found enricher after line 91.
And shall we change the name of `includedEnrichers` to `enrichersToLoad`.
From the perspective of naming, the `includedEnrichers` is meaning the
enrichers that user configured, and it shouldn't be modified. But the
`enrichersToLoad` means the enrichers that should be load, when some of it is
loaded, it could be removed from this set since it's not need load any more.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
