You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/03/16 01:51:33 UTC
[GitHub] [kafka] C0urante opened a new pull request #11903: KAFKA-13497: Add trace logging to RegexRouter
C0urante opened a new pull request #11903:
URL: https://github.com/apache/kafka/pull/11903
[JIRA](https://issues.apache.org/jira/browse/KAFKA-13497)
Adds runtime logging to the `RegexRouter` to show exactly which topics get routed where.
As noted in the Jira ticket, this is similar to existing logging in the `Cast` SMT: https://github.com/apache/kafka/blob/620f1d88d80fdf8150bd0b75be307bc4a2d3a0ea/connect/transforms/src/main/java/org/apache/kafka/connect/transforms/Cast.java#L174
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11903: KAFKA-13497: Add trace logging to RegexRouter
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11903:
URL: https://github.com/apache/kafka/pull/11903#discussion_r829300000
##########
File path: connect/transforms/src/main/java/org/apache/kafka/connect/transforms/RegexRouter.java
##########
@@ -57,7 +61,10 @@ public R apply(R record) {
final Matcher matcher = regex.matcher(record.topic());
if (matcher.matches()) {
final String topic = matcher.replaceFirst(replacement);
+ log.trace("Rerouting from topic '{}' to new topic '{}'", record.topic(), topic);
Review comment:
Should we gate it with `log.isTraceEnabled()`?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac merged pull request #11903: KAFKA-13497: Add trace logging to RegexRouter
Posted by GitBox <gi...@apache.org>.
dajac merged pull request #11903:
URL: https://github.com/apache/kafka/pull/11903
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11903: KAFKA-13497: Add trace logging to RegexRouter
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11903:
URL: https://github.com/apache/kafka/pull/11903#discussion_r829300000
##########
File path: connect/transforms/src/main/java/org/apache/kafka/connect/transforms/RegexRouter.java
##########
@@ -57,7 +61,10 @@ public R apply(R record) {
final Matcher matcher = regex.matcher(record.topic());
if (matcher.matches()) {
final String topic = matcher.replaceFirst(replacement);
+ log.trace("Rerouting from topic '{}' to new topic '{}'", record.topic(), topic);
Review comment:
Should we gate it with `log.isTraceEnabled()`?
##########
File path: connect/transforms/src/main/java/org/apache/kafka/connect/transforms/RegexRouter.java
##########
@@ -57,7 +61,10 @@ public R apply(R record) {
final Matcher matcher = regex.matcher(record.topic());
if (matcher.matches()) {
final String topic = matcher.replaceFirst(replacement);
+ log.trace("Rerouting from topic '{}' to new topic '{}'", record.topic(), topic);
Review comment:
You're right. I misread the line and thought that we were constructing the string before calling trace.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac merged pull request #11903: KAFKA-13497: Add trace logging to RegexRouter
Posted by GitBox <gi...@apache.org>.
dajac merged pull request #11903:
URL: https://github.com/apache/kafka/pull/11903
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a change in pull request #11903: KAFKA-13497: Add trace logging to RegexRouter
Posted by GitBox <gi...@apache.org>.
C0urante commented on a change in pull request #11903:
URL: https://github.com/apache/kafka/pull/11903#discussion_r829392255
##########
File path: connect/transforms/src/main/java/org/apache/kafka/connect/transforms/RegexRouter.java
##########
@@ -57,7 +61,10 @@ public R apply(R record) {
final Matcher matcher = regex.matcher(record.topic());
if (matcher.matches()) {
final String topic = matcher.replaceFirst(replacement);
+ log.trace("Rerouting from topic '{}' to new topic '{}'", record.topic(), topic);
Review comment:
I don't see a reason to.
If trace-level logging is disabled, the cost of this line will be two method invocations (one for `Record::topic`, which has a [trivial implementation](https://github.com/apache/kafka/blob/7afdb069bf5539ec404d9305239849ac35ad2d82/connect/api/src/main/java/org/apache/kafka/connect/connector/ConnectRecord.java#L69-L71), and one for `Logger::trace`), plus a check to see if trace-level logging is enabled. No message formatting will take place.
If we wrap this with `Logger::isTraceEnabled`, we'll reduce that to a single method invocation and check to see if trace-level logging is enabled. But if trace-level logging is enabled, we'd end up doubling the checks to see if the logger is enabled, and add an extra method invocation. There's a good writeup on the performance implications of this style of checking [here](https://logging.apache.org/log4j/1.2/manual.html#performance).
It seems unlikely that any of this will have a significant impact on performance, especially considering that this same code path already contains a regex check a few lines above. There's also the existing logging in `Cast` that's mentioned in the description that takes place at trace level and doesn't have a guard for `Logger::isTraceEnabled`.
Ultimately, if we're concerned about performance here, it'd probably be much more effective to cache the mappings of input topic -> output topic than to add guards around trace-level log statements that don't do involve any string concatenation or expensive `toString` calls.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11903: KAFKA-13497: Add trace logging to RegexRouter
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11903:
URL: https://github.com/apache/kafka/pull/11903#discussion_r829826219
##########
File path: connect/transforms/src/main/java/org/apache/kafka/connect/transforms/RegexRouter.java
##########
@@ -57,7 +61,10 @@ public R apply(R record) {
final Matcher matcher = regex.matcher(record.topic());
if (matcher.matches()) {
final String topic = matcher.replaceFirst(replacement);
+ log.trace("Rerouting from topic '{}' to new topic '{}'", record.topic(), topic);
Review comment:
You're right. I misread the line and thought that we were constructing the string before calling trace.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a change in pull request #11903: KAFKA-13497: Add trace logging to RegexRouter
Posted by GitBox <gi...@apache.org>.
C0urante commented on a change in pull request #11903:
URL: https://github.com/apache/kafka/pull/11903#discussion_r829392255
##########
File path: connect/transforms/src/main/java/org/apache/kafka/connect/transforms/RegexRouter.java
##########
@@ -57,7 +61,10 @@ public R apply(R record) {
final Matcher matcher = regex.matcher(record.topic());
if (matcher.matches()) {
final String topic = matcher.replaceFirst(replacement);
+ log.trace("Rerouting from topic '{}' to new topic '{}'", record.topic(), topic);
Review comment:
I don't see a reason to.
If trace-level logging is disabled, the cost of this line will be two method invocations (one for `Record::topic`, which has a [trivial implementation](https://github.com/apache/kafka/blob/7afdb069bf5539ec404d9305239849ac35ad2d82/connect/api/src/main/java/org/apache/kafka/connect/connector/ConnectRecord.java#L69-L71), and one for `Logger::trace`), plus a check to see if trace-level logging is enabled. No message formatting will take place.
If we wrap this with `Logger::isTraceEnabled`, we'll reduce that to a single method invocation and check to see if trace-level logging is enabled. But if trace-level logging is enabled, we'd end up doubling the checks to see if the logger is enabled, and add an extra method invocation. There's a good writeup on the performance implications of this style of checking [here](https://logging.apache.org/log4j/1.2/manual.html#performance).
It seems unlikely that any of this will have a significant impact on performance, especially considering that this same code path already contains a regex check a few lines above. There's also the existing logging in `Cast` that's mentioned in the description that takes place at trace level and doesn't have a guard for `Logger::isTraceEnabled`.
Ultimately, if we're concerned about performance here, it'd probably be much more effective to cache the mappings of input topic -> output topic than to add guards around trace-level log statements that don't do involve any string concatenation or expensive `toString` calls.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org