peter-toth commented on code in PR #833:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/833#discussion_r4029048018
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ConfigurableEventRecorder.java:
##########
@@ -94,6 +102,24 @@ private static boolean eventsEnabled() {
return Boolean.TRUE.equals(KUBERNETES_EVENTS_ENABLED.getValue());
}
+ private static boolean isExcluded(String reason) {
+ // A null resolved value is treated as an empty list.
+ return
Utils.sanitizeCommaSeparatedStrAsSet(KUBERNETES_EVENTS_EXCLUDED_REASONS.getValue())
Review Comment:
**Finding 1.** This helper carries a sentinel that belongs to a different
option. `Utils.java:71-73`:
```java
if ("*".equals(str)) {
return Set.of();
}
```
That is right for `OPERATOR_WATCHED_NAMESPACES` (`Utils.java:202`), where
`*` means "watch everything, so do not restrict". Here the same branch empties
the exclusion list, so the whole value `*` excludes nothing. I confirmed both
halves with a probe test:
```java
setExcludedReasons("*");
recorder.record(EventRecord.normal("RunningHealthy", "published?"),
context);
verify(delegate).record(event, context); // passes: * excludes
nothing
setExcludedReasons(".*");
recorder.record(EventRecord.normal("RunningHealthy", "dropped?"), context);
verifyNoInteractions(delegate); // passes: .* excludes
everything
```
The end state is not wrong against the docs - `*` is not a valid Java regex,
and the description already says an invalid expression "matches only the reason
identical to it", which also excludes nothing. What is lost is the signal: the
sentinel returns before `matches` ever runs, so `*` is the one malformed value
that produces no warning, and a user who reaches for the obvious wildcard gets
silence. It also leaves this option's semantics dependent on a helper whose `*`
case exists for another feature.
Splitting locally keeps the documented rule as the only rule:
```java
private static boolean isExcluded(String reason) {
// A null or blank resolved value is treated as an empty list. The comma
splitting is local
// rather than Utils.sanitizeCommaSeparatedStrAsSet, whose "*" sentinel
means "no restriction"
// for the watched-namespaces option and would silently empty this list
instead.
String value = KUBERNETES_EVENTS_EXCLUDED_REASONS.getValue();
if (StringUtils.isBlank(value)) {
return false;
}
return Arrays.stream(value.split(","))
.map(String::trim)
.filter(StringUtils::isNotBlank)
.anyMatch(regex -> matches(regex, reason));
}
```
If you would rather keep the helper, a test pinning `*` plus a sentence next
to the `,` caveat at `docs/configuration.md:92` would at least make the
behaviour discoverable.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
