davsclaus commented on code in PR #24985:
URL: https://github.com/apache/camel/pull/24985#discussion_r3649984608
##########
core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/DefaultTracer.java:
##########
@@ -347,7 +352,7 @@ protected void dumpTrace(String out, Object node) {
}
}
- protected boolean shouldTracePattern(NamedNode definition) {
+ protected boolean shouldTracePattern(NamedNode definition, String[]
patterns) {
Review Comment:
This is a `protected` method in a public non-final class — oscerd's review
dismissed this as `private`, but it is `protected` on `main` (line 350). The
signature change from `shouldTracePattern(NamedNode)` to
`shouldTracePattern(NamedNode, String[])` is a source/binary compatibility
break for any downstream code extending `DefaultTracer` and overriding this
method.
No known subclasses exist in the Camel codebase, and the class is in
`camel-base-engine` (internal module), so practical risk is very low. Just
flagging that the concern was dismissed on incorrect grounds.
##########
core/camel-base-engine/src/main/java/org/apache/camel/impl/debugger/BacklogTracer.java:
##########
@@ -478,37 +484,41 @@ public void setTraceTemplates(boolean traceTemplates) {
@Override
public String getTracePattern() {
- return tracePattern;
+ TracePatternHolder ph = tracePatternHolder;
+ return ph != null ? ph.tracePattern() : null;
}
@Override
public void setTracePattern(String tracePattern) {
- this.tracePattern = tracePattern;
if (tracePattern != null) {
// the pattern can have multiple nodes separated by comma
- this.patterns = tracePattern.split(",");
+ this.tracePatternHolder = new TracePatternHolder(tracePattern,
tracePattern.split(","));
} else {
- this.patterns = null;
+ this.tracePatternHolder = null;
}
}
@Override
public String getTraceFilter() {
- return traceFilter;
+ TraceFilterHolder fh = traceFilterHolder;
+ return fh != null ? fh.traceFilter() : null;
}
@Override
public void setTraceFilter(String filter) {
- this.traceFilter = filter;
if (filter != null) {
// assume simple language
+ Predicate p;
String name = StringHelper.before(filter, ":");
if (name != null) {
- predicate =
camelContext.resolveLanguage(name).createPredicate(filter);
+ p = camelContext.resolveLanguage(name).createPredicate(filter);
} else {
// use simple language by default
- predicate = simple.createPredicate(filter);
+ p = simple.createPredicate(filter);
}
+ this.traceFilterHolder = new TraceFilterHolder(filter, p);
+ } else {
+ this.traceFilterHolder = null;
Review Comment:
This `else` branch fixes a pre-existing bug: the old code set `traceFilter =
null` but never cleared `predicate`, so `shouldTrace()` would keep evaluating a
stale predicate after the user cleared the filter via JMX. The fix is correct.
Since this is a deterministic behavior change (not just a visibility fix),
it should have a test — e.g. set a filter, verify it's applied, clear it with
`setTraceFilter(null)`, verify filtering is disabled.
--
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]
