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/04/08 18:18:03 UTC
[GitHub] [kafka] vvcephei opened a new pull request, #12020: MINOR: Add generation to consumer assignor logs
vvcephei opened a new pull request, #12020:
URL: https://github.com/apache/kafka/pull/12020
Reading assignor logs is really confusing in large part because they are
spread across different layers of abstraction (the ConsumerCoordinator
and the ConsumerPartitionAssignor, which in Streams consists of several
layers of its own). Each layer in the abstraction reports useful information
that only it has access to, but because they are split over multiple lines, with
multiple members in the cluster, and (often) multiple rebalances taking place
in rapid succession, it's often hard to understand which logs are part of
which rebalance.
This PR attempts to improve the situation by adding a common prefix to
all assignment logs that includes the current generation number. Then,
debuggers can pull out all logs for a particular rebalance with a simple
`grep "generationId=5"`. In order to add this useful context without violating
encapsulation, the PR simply adds a generic "dynamic log context" that
assignors can optionally participate in (by mixing in `ContextualLogging`).
I chose to keep these changes restricted to private APIs for the moment.
If this approach proves useful, we can propose a KIP at a later date to
make the APIs public.
### 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] vvcephei commented on a diff in pull request #12020: MINOR: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
vvcephei commented on code in PR #12020:
URL: https://github.com/apache/kafka/pull/12020#discussion_r847561327
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamsPartitionAssignor.java:
##########
@@ -86,10 +87,23 @@
import static org.apache.kafka.streams.processor.internals.assignment.StreamsAssignmentProtocolVersions.UNKNOWN;
import static org.apache.kafka.streams.processor.internals.assignment.SubscriptionInfo.UNKNOWN_OFFSET_SUM;
-public class StreamsPartitionAssignor implements ConsumerPartitionAssignor, Configurable {
+public class StreamsPartitionAssignor implements ConsumerPartitionAssignor, Configurable,
+ ContextualLogging {
Review Comment:
Yeah, I was struggling to name the mixin nicely (the `Configurable` naming pattern would yield something silly like `ContexuallyLoggable`). After mulling on it, I think I'll change it to `DynamicLoggingContextMixin`. It's an internal interface for now, so I think simple and clear is more important than pretty for now.
--
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] mjsax commented on a diff in pull request #12020: MINOR: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
mjsax commented on code in PR #12020:
URL: https://github.com/apache/kafka/pull/12020#discussion_r846536809
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamsPartitionAssignor.java:
##########
@@ -86,10 +87,23 @@
import static org.apache.kafka.streams.processor.internals.assignment.StreamsAssignmentProtocolVersions.UNKNOWN;
import static org.apache.kafka.streams.processor.internals.assignment.SubscriptionInfo.UNKNOWN_OFFSET_SUM;
-public class StreamsPartitionAssignor implements ConsumerPartitionAssignor, Configurable {
+public class StreamsPartitionAssignor implements ConsumerPartitionAssignor, Configurable,
+ ContextualLogging {
Review Comment:
Isn't it a little weird that the assignor implement that contextual logging interface? It's an assignor, not a logger?
--
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] mjsax commented on pull request #12020: MINOR: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
mjsax commented on PR #12020:
URL: https://github.com/apache/kafka/pull/12020#issuecomment-1093473885
Build failed. Looks concerning?
--
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 diff in pull request #12020: KAFKA-13818: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12020:
URL: https://github.com/apache/kafka/pull/12020#discussion_r969868569
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinator.java:
##########
@@ -164,13 +166,23 @@ public ConsumerCoordinator(GroupRebalanceConfig rebalanceConfig,
metricGrpPrefix,
time);
this.rebalanceConfig = rebalanceConfig;
- this.log = logContext.logger(ConsumerCoordinator.class);
+ final Supplier<String> dynamicPrefix =
+ () -> logContext.logPrefix() + "[generationId=" + generation().generationId + "] ";
+ this.log = new DynamicPrefixLogger(
+ dynamicPrefix,
+ LoggerFactory.getLogger(ConsumerCoordinator.class)
+ );
Review Comment:
Thanks @vvcephei--I'm personally a little skeptical that one extra method invocation is going to have a significant performance impact for logging, even broker-side, but I don't mind erring on the side of caution here for now. I've closed my PR against this branch; we can always revisit later.
I do still wonder about modifying the logger used by the `AbstractCoordinator` superclass, since that's (AFAIK) only used client-side?
--
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 diff in pull request #12020: MINOR: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12020:
URL: https://github.com/apache/kafka/pull/12020#discussion_r846657763
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinator.java:
##########
@@ -164,13 +166,23 @@ public ConsumerCoordinator(GroupRebalanceConfig rebalanceConfig,
metricGrpPrefix,
time);
this.rebalanceConfig = rebalanceConfig;
- this.log = logContext.logger(ConsumerCoordinator.class);
+ final Supplier<String> dynamicPrefix =
+ () -> logContext.logPrefix() + "[generationId=" + generation().generationId + "] ";
+ this.log = new DynamicPrefixLogger(
+ dynamicPrefix,
+ LoggerFactory.getLogger(ConsumerCoordinator.class)
+ );
Review Comment:
Is it worth trying to modify the logger used by the superclass as well?
--
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
Re: [PR] KAFKA-13818: Add generation to consumer assignor logs [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12020:
URL: https://github.com/apache/kafka/pull/12020#issuecomment-2562119517
This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.
<p>
If you are having difficulty finding a reviewer, please reach out on the
[mailing list](https://kafka.apache.org/contact).
<p>
If this PR is no longer valid or desired, please feel free to close it. If no activity
occurs in the next 30 days, it will be automatically closed.
--
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
Re: [PR] KAFKA-13818: Add generation to consumer assignor logs [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12020:
URL: https://github.com/apache/kafka/pull/12020#issuecomment-2614195400
This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.
--
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 diff in pull request #12020: KAFKA-13818: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12020:
URL: https://github.com/apache/kafka/pull/12020#discussion_r969868569
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinator.java:
##########
@@ -164,13 +166,23 @@ public ConsumerCoordinator(GroupRebalanceConfig rebalanceConfig,
metricGrpPrefix,
time);
this.rebalanceConfig = rebalanceConfig;
- this.log = logContext.logger(ConsumerCoordinator.class);
+ final Supplier<String> dynamicPrefix =
+ () -> logContext.logPrefix() + "[generationId=" + generation().generationId + "] ";
+ this.log = new DynamicPrefixLogger(
+ dynamicPrefix,
+ LoggerFactory.getLogger(ConsumerCoordinator.class)
+ );
Review Comment:
Thanks @vvcephei--I'm personally a little skeptical that one extra method invocation is going to have a large performance impact for logging, even broker-side, but I don't mind erring on the side of caution here for now.
I do still wonder about modifying the logger used by the `AbstractCoordinator` superclass, since that's (AFAIK) only used client-side?
--
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] vvcephei commented on pull request #12020: KAFKA-13818: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
vvcephei commented on PR #12020:
URL: https://github.com/apache/kafka/pull/12020#issuecomment-1254308832
Note: I might want to adjust the approach here, as in a subsequent investigation, I have realized it would be quite nice to prefix the cluster membership lines with the generation id as well, but we would not want to include the entire log prefix on each line (I don't think we would anyway).
See https://issues.apache.org/jira/browse/KAFKA-14253 for the cluster member lines I'm thinking of.
--
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] vvcephei commented on a diff in pull request #12020: KAFKA-13818: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
vvcephei commented on code in PR #12020:
URL: https://github.com/apache/kafka/pull/12020#discussion_r962326811
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinator.java:
##########
@@ -164,13 +166,23 @@ public ConsumerCoordinator(GroupRebalanceConfig rebalanceConfig,
metricGrpPrefix,
time);
this.rebalanceConfig = rebalanceConfig;
- this.log = logContext.logger(ConsumerCoordinator.class);
+ final Supplier<String> dynamicPrefix =
+ () -> logContext.logPrefix() + "[generationId=" + generation().generationId + "] ";
+ this.log = new DynamicPrefixLogger(
+ dynamicPrefix,
+ LoggerFactory.getLogger(ConsumerCoordinator.class)
+ );
Review Comment:
Thanks, @C0urante !
I think for now, I'd like to keep the change on the client side of the protocol only. The main intent of this change is to make it easier to see what's happening with rebalances on the client side, and I'm also a little concerned about the performance impact of this code on the broker.
Perf is not as concerning on the client side because a single consumer group spends most of its time processing and only occasionally performs a rebalance. In contrast, a broker managing a lot of consumer groups may spend a lot more time logging these messages.
--
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] vvcephei commented on a diff in pull request #12020: MINOR: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
vvcephei commented on code in PR #12020:
URL: https://github.com/apache/kafka/pull/12020#discussion_r846375420
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/DynamicPrefixLogger.java:
##########
@@ -0,0 +1,353 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.clients.consumer.internals;
+
+import org.slf4j.Logger;
+import org.slf4j.Marker;
+
+import java.util.function.Supplier;
+
+public final class DynamicPrefixLogger implements Logger {
+
+ private final Supplier<String> prefix;
+ private final Logger delegate;
+
+ public DynamicPrefixLogger(final Supplier<String> prefix, final Logger delegate) {
+ this.prefix = prefix;
+ this.delegate = delegate;
+ }
+
+ @Override
+ public String getName() {
+ return delegate.getName();
+ }
+
+ @Override
+ public boolean isTraceEnabled() {
+ return delegate.isTraceEnabled();
+ }
+
+ @Override
+ public void trace(final String msg) {
+ delegate.trace(prefix.get() + msg);
Review Comment:
TODO: if this proposal passes the sniff test, I need to bracket all these delegate calls with `if (isXEnabled())`.
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/DynamicPrefixLogger.java:
##########
@@ -0,0 +1,353 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.clients.consumer.internals;
+
+import org.slf4j.Logger;
+import org.slf4j.Marker;
+
+import java.util.function.Supplier;
+
+public final class DynamicPrefixLogger implements Logger {
Review Comment:
This is similar to the LoggingContext logger, but I didn't want to mess with that one, since adding the dynamic Supplier evaluation will be nonzero cost and the LoggingContext is more widely used.
--
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 diff in pull request #12020: MINOR: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12020:
URL: https://github.com/apache/kafka/pull/12020#discussion_r846659580
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinator.java:
##########
@@ -164,13 +166,23 @@ public ConsumerCoordinator(GroupRebalanceConfig rebalanceConfig,
metricGrpPrefix,
time);
this.rebalanceConfig = rebalanceConfig;
- this.log = logContext.logger(ConsumerCoordinator.class);
+ final Supplier<String> dynamicPrefix =
+ () -> logContext.logPrefix() + "[generationId=" + generation().generationId + "] ";
+ this.log = new DynamicPrefixLogger(
+ dynamicPrefix,
+ LoggerFactory.getLogger(ConsumerCoordinator.class)
+ );
Review Comment:
If we don't want to modify `AbstractCoordinator` to include generation IDs for all of its subclasses instead of just `ConsumerCoordinator`, we could modify `AbstractCoordinator` to also implement `ContextualLogging` and then invoke `super::setLoggingContext` in the `ConsumerCoordinator` constructor.
--
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] mjsax commented on a diff in pull request #12020: MINOR: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
mjsax commented on code in PR #12020:
URL: https://github.com/apache/kafka/pull/12020#discussion_r846536809
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamsPartitionAssignor.java:
##########
@@ -86,10 +87,23 @@
import static org.apache.kafka.streams.processor.internals.assignment.StreamsAssignmentProtocolVersions.UNKNOWN;
import static org.apache.kafka.streams.processor.internals.assignment.SubscriptionInfo.UNKNOWN_OFFSET_SUM;
-public class StreamsPartitionAssignor implements ConsumerPartitionAssignor, Configurable {
+public class StreamsPartitionAssignor implements ConsumerPartitionAssignor, Configurable,
+ ContextualLogging {
Review Comment:
Isn't it a little weird that the assignor implement that contextual logging interface? It's an assignor, not a logger? (Maybe it's just the name of the interface.)
--
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 diff in pull request #12020: KAFKA-13818: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12020:
URL: https://github.com/apache/kafka/pull/12020#discussion_r969868569
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinator.java:
##########
@@ -164,13 +166,23 @@ public ConsumerCoordinator(GroupRebalanceConfig rebalanceConfig,
metricGrpPrefix,
time);
this.rebalanceConfig = rebalanceConfig;
- this.log = logContext.logger(ConsumerCoordinator.class);
+ final Supplier<String> dynamicPrefix =
+ () -> logContext.logPrefix() + "[generationId=" + generation().generationId + "] ";
+ this.log = new DynamicPrefixLogger(
+ dynamicPrefix,
+ LoggerFactory.getLogger(ConsumerCoordinator.class)
+ );
Review Comment:
Thanks @vvcephei--I'm personally a little skeptical that one extra method invocation is going to have a significant performance impact for logging, even broker-side, but I don't mind erring on the side of caution here for now.
I do still wonder about modifying the logger used by the `AbstractCoordinator` superclass, since that's (AFAIK) only used client-side?
--
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 diff in pull request #12020: KAFKA-13818: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12020:
URL: https://github.com/apache/kafka/pull/12020#discussion_r969868569
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinator.java:
##########
@@ -164,13 +166,23 @@ public ConsumerCoordinator(GroupRebalanceConfig rebalanceConfig,
metricGrpPrefix,
time);
this.rebalanceConfig = rebalanceConfig;
- this.log = logContext.logger(ConsumerCoordinator.class);
+ final Supplier<String> dynamicPrefix =
+ () -> logContext.logPrefix() + "[generationId=" + generation().generationId + "] ";
+ this.log = new DynamicPrefixLogger(
+ dynamicPrefix,
+ LoggerFactory.getLogger(ConsumerCoordinator.class)
+ );
Review Comment:
Thanks @vvcephei--I'm personally a little skeptical that one stack frame is going to have a large performance impact for logging, even broker-side, but I don't mind erring on the side of caution here for now.
I do still wonder about modifying the logger used by the `AbstractCoordinator` superclass, since that's (AFAIK) only used client-side?
--
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 pull request #12020: MINOR: Add generation to consumer assignor logs
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12020:
URL: https://github.com/apache/kafka/pull/12020#issuecomment-1094090371
Thanks John, agree that generation ID would be useful to add to help group messages from different loggers together. The duplication and boilerplate in here don't seem great, though--I've opened up https://github.com/apache/kafka/pull/12023 as a PR against this branch to take a stab at addressing that. What do you think?
--
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
Re: [PR] KAFKA-13818: Add generation to consumer assignor logs [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #12020: KAFKA-13818: Add generation to consumer assignor logs
URL: https://github.com/apache/kafka/pull/12020
--
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