Re: [PR] MINOR: remove unnecessary logging [kafka]
jolshan commented on PR #15396: URL: https://github.com/apache/kafka/pull/15396#issuecomment-1960482583 fyi: https://github.com/apache/kafka/pull/15422 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: remove unnecessary logging [kafka]
jolshan commented on PR #15396: URL: https://github.com/apache/kafka/pull/15396#issuecomment-1960438257 Hey this broke the following tests that expect this log message: ``` RocksDBSegmentedBytesStoreTest. shouldLogAndMeasureExpiredRecords[org.apache.kafka.streams.state.internals.SessionKeySchema@788fcafb] RocksDBSegmentedBytesStoreTest. shouldLogAndMeasureExpiredRecords[org.apache.kafka.streams.state.internals.WindowKeySchema@4febb875] RocksDBSessionStoreTest. shouldLogAndMeasureExpiredRecords[RocksDBSessionStore] RocksDBTimestampedSegmentedBytesStoreTest. shouldLogAndMeasureExpiredRecords[org.apache.kafka.streams.state.internals.SessionKeySchema@395b56bb] RocksDBTimestampedSegmentedBytesStoreTest. shouldLogAndMeasureExpiredRecords[org.apache.kafka.streams.state.internals.WindowKeySchema@256f8274] RocksDBWindowStoreTest. shouldLogAndMeasureExpiredRecords[RocksDBWindowStore] ``` For some reason these tests don't retry and trunk is now red. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: remove unnecessary logging [kafka]
mjsax merged PR #15396: URL: https://github.com/apache/kafka/pull/15396 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: remove unnecessary logging [kafka]
wcarlson5 commented on code in PR #15396: URL: https://github.com/apache/kafka/pull/15396#discussion_r1496604182 ## streams/src/main/java/org/apache/kafka/streams/state/internals/AbstractRocksDBSegmentedBytesStore.java: ## @@ -264,7 +264,6 @@ public void put(final Bytes key, final S segment = segments.getOrCreateSegmentIfLive(segmentId, context, observedStreamTime); if (segment == null) { expiredRecordSensor.record(1.0d, context.currentSystemTimeMs()); -LOG.warn("Skipping record for expired segment."); Review Comment: I'm indifferent. We can convert it if anyone finds value in it but I don't really see it. But I also don't think it will crowd the debug logs too much either. 🤷 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: remove unnecessary logging [kafka]
mjsax commented on code in PR #15396: URL: https://github.com/apache/kafka/pull/15396#discussion_r1496379447 ## streams/src/main/java/org/apache/kafka/streams/state/internals/AbstractRocksDBSegmentedBytesStore.java: ## @@ -264,7 +264,6 @@ public void put(final Bytes key, final S segment = segments.getOrCreateSegmentIfLive(segmentId, context, observedStreamTime); if (segment == null) { expiredRecordSensor.record(1.0d, context.currentSystemTimeMs()); -LOG.warn("Skipping record for expired segment."); Review Comment: I don't see much value? We don't log any detailed information... -- the log line only gives you a processing time timestamp but nothing more... If we would want to make it useful, we could add offset/timestamp metadata (but not log key/value as we never log data...) -- for this case, it might be useful to keep at DEBUG or TRACE. Let's hear from others about 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: remove unnecessary logging [kafka]
kpatelatwork commented on code in PR #15396: URL: https://github.com/apache/kafka/pull/15396#discussion_r1496373291 ## streams/src/main/java/org/apache/kafka/streams/state/internals/AbstractRocksDBSegmentedBytesStore.java: ## @@ -264,7 +264,6 @@ public void put(final Bytes key, final S segment = segments.getOrCreateSegmentIfLive(segmentId, context, observedStreamTime); if (segment == null) { expiredRecordSensor.record(1.0d, context.currentSystemTimeMs()); -LOG.warn("Skipping record for expired segment."); Review Comment: is there a value in converting to debug instead of removing 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] MINOR: remove unnecessary logging [kafka]
mjsax opened a new pull request, #15396: URL: https://github.com/apache/kafka/pull/15396 We already record dropping record via metrics and logging at WARN level is too noise. This PR removes the unnecessary logging. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org