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/12/13 09:47:47 UTC
[GitHub] [kafka] vamossagar12 opened a new pull request, #12985: KAFKA-13602: Remove unwanted logging in RecordCollectorImpl.java
vamossagar12 opened a new pull request, #12985:
URL: https://github.com/apache/kafka/pull/12985
There is unwanted logging introduced by https://github.com/apache/kafka/pull/12803 as pointed out in this comment: https://github.com/apache/kafka/pull/12803#discussion_r1046852940. This PR removes it.
--
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] cadonna merged pull request #12985: KAFKA-13602: Remove unwanted logging in RecordCollectorImpl.java
Posted by GitBox <gi...@apache.org>.
cadonna merged PR #12985:
URL: https://github.com/apache/kafka/pull/12985
--
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] vamossagar12 commented on pull request #12985: KAFKA-13602: Remove unwanted logging in RecordCollectorImpl.java
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12985:
URL: https://github.com/apache/kafka/pull/12985#issuecomment-1348487061
Thanks @cadonna . I changed the verbiage of the log line to not include key/value.
--
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] cadonna commented on a diff in pull request #12985: KAFKA-13602: Remove unwanted logging in RecordCollectorImpl.java
Posted by GitBox <gi...@apache.org>.
cadonna commented on code in PR #12985:
URL: https://github.com/apache/kafka/pull/12985#discussion_r1052198261
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/RecordCollectorImpl.java:
##########
@@ -159,7 +159,8 @@ public <K, V> void send(final String topic,
final Set<Integer> multicastPartitions = maybeMulticastPartitions.get();
if (multicastPartitions.isEmpty()) {
// If a record is not to be sent to any partition, mark it as a dropped record.
- log.debug("Not sending the record with key {} , value {} to any partition", key, value);
+ log.warn("Skipping record as partitioner returned empty partitions. "
Review Comment:
I think, we should never log keys and values on any log level. Since the provider of the Streams service might not own the data, but might be able to change the log level, having keys and values in the logs seems a security risk to me.
Regarding, debug vs. warn vs. trace here. In other places, we use warn log messages when records are dropped since the dropped records metric does not contain any information about why the record was dropped. However, in this case I understand -- and correct me if I am wrong -- that the partitioner might drop records on purpose. So having a warn log message for each record seems not appropriate. Still it would be great to have some kind of logging. Maybe some regular summary (e.g., for the first 10 records and then every 100 records and then on larger intervals?) or indeed logging at trace level.
--
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] ableegoldman commented on a diff in pull request #12985: KAFKA-13602: Remove unwanted logging in RecordCollectorImpl.java
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on code in PR #12985:
URL: https://github.com/apache/kafka/pull/12985#discussion_r1050225672
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/RecordCollectorImpl.java:
##########
@@ -159,7 +159,8 @@ public <K, V> void send(final String topic,
final Set<Integer> multicastPartitions = maybeMulticastPartitions.get();
if (multicastPartitions.isEmpty()) {
// If a record is not to be sent to any partition, mark it as a dropped record.
- log.debug("Not sending the record with key {} , value {} to any partition", key, value);
+ log.warn("Skipping record as partitioner returned empty partitions. "
Review Comment:
Imo we shouldn't put anything that can potentially be logged for every record above `TRACE` as a rule of thumb. We always get user complaints and have to downgrade the logging level for these sort of things anyways. I would suggest to go back to including the key and value but put it at TRACE instead.
Thoughts? @cadonna
--
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] ableegoldman commented on pull request #12985: KAFKA-13602: Remove unwanted logging in RecordCollectorImpl.java
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on PR #12985:
URL: https://github.com/apache/kafka/pull/12985#issuecomment-1353835646
Cherrypicked to 3.4
--
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] cadonna commented on pull request #12985: KAFKA-13602: Remove unwanted logging in RecordCollectorImpl.java
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12985:
URL: https://github.com/apache/kafka/pull/12985#issuecomment-1348818930
Test failures are unrelated:
```
Build / JDK 8 and Scala 2.12 / org.apache.kafka.clients.consumer.StickyAssignorTest.testLargeAssignmentAndGroupWithNonEqualSubscription()
Build / JDK 8 and Scala 2.12 / org.apache.kafka.common.security.oauthbearer.internals.secured.RefreshingHttpsJwksTest.testSecondaryRefreshAfterElapsedDelay()
Build / JDK 8 and Scala 2.12 / integration.kafka.server.FetchFromFollowerIntegrationTest.testFetchFromFollowerWithRoll()
Build / JDK 8 and Scala 2.12 / integration.kafka.server.FetchFromFollowerIntegrationTest.testFetchFromFollowerWithRoll()
Build / JDK 8 and Scala 2.12 / org.apache.kafka.streams.integration.SmokeTestDriverIntegrationTest.[1] true
Build / JDK 8 and Scala 2.12 / org.apache.kafka.streams.integration.SmokeTestDriverIntegrationTest.[2] false
Build / JDK 17 and Scala 2.13 / integration.kafka.server.FetchFromFollowerIntegrationTest.testFetchFromFollowerWithRoll()
Build / JDK 17 and Scala 2.13 / integration.kafka.server.FetchFromFollowerIntegrationTest.testFetchFromFollowerWithRoll()
Build / JDK 17 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testConfigurationOperations()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.clients.consumer.KafkaConsumerTest.testShouldAttemptToRejoinGroupAfterSyncGroupFailed()
Build / JDK 11 and Scala 2.13 / integration.kafka.server.FetchFromFollowerIntegrationTest.testFetchFromFollowerWithRoll()
Build / JDK 11 and Scala 2.13 / integration.kafka.server.FetchFromFollowerIntegrationTest.testFetchFromFollowerWithRoll()
Build / JDK 11 and Scala 2.13 / kafka.api.UserQuotaTest.testProducerConsumerOverrideLowerQuota(String).quorum=kraft
Build / JDK 11 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testUnregisterBroker()
```
--
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] ableegoldman commented on a diff in pull request #12985: KAFKA-13602: Remove unwanted logging in RecordCollectorImpl.java
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on code in PR #12985:
URL: https://github.com/apache/kafka/pull/12985#discussion_r1059241223
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/RecordCollectorImpl.java:
##########
@@ -159,7 +159,8 @@ public <K, V> void send(final String topic,
final Set<Integer> multicastPartitions = maybeMulticastPartitions.get();
if (multicastPartitions.isEmpty()) {
// If a record is not to be sent to any partition, mark it as a dropped record.
- log.debug("Not sending the record with key {} , value {} to any partition", key, value);
+ log.warn("Skipping record as partitioner returned empty partitions. "
Review Comment:
My impression was that it's been standard practice to allow logging keys/values, but only at the TRACE level -- might be worth checking with the security team?
I guess my feeling here is that this definitely shouldn't be logged at WARN for every record, but could be done as a regular summary (a practice I think we should be adopting more often than we have in the past) or as I said, at TRACE. I would favor the summary unless we can include the key/value as a TRACE log, which again we should try to confirm the standard here. I suppose it's useful to know that records are being dropped due to the partitioner, which makes the WARN summary useful, but I do wonder if users might still have trouble debugging this partitioner choice without access to the key/values it's dropping?
Thoughts?
--
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] vamossagar12 commented on a diff in pull request #12985: KAFKA-13602: Remove unwanted logging in RecordCollectorImpl.java
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on code in PR #12985:
URL: https://github.com/apache/kafka/pull/12985#discussion_r1050389751
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/RecordCollectorImpl.java:
##########
@@ -159,7 +159,8 @@ public <K, V> void send(final String topic,
final Set<Integer> multicastPartitions = maybeMulticastPartitions.get();
if (multicastPartitions.isEmpty()) {
// If a record is not to be sent to any partition, mark it as a dropped record.
- log.debug("Not sending the record with key {} , value {} to any partition", key, value);
+ log.warn("Skipping record as partitioner returned empty partitions. "
Review Comment:
@ableegoldman my two cents , i think it's safer to not log keys and values as those can contain sensitive information at times and can lead to security issues.
--
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