Re: [PR] MINOR: remove unnecessary logging [kafka]

[via GitHub]

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]

[via GitHub]

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]

[via GitHub]

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]

[via GitHub]

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]

[via GitHub]

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]

[via GitHub]

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