You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "danicafine (via GitHub)" <gi...@apache.org> on 2024/07/27 17:06:13 UTC
[PR] KAFKA-17109: Reduce log message load for failed locking [kafka]
danicafine opened a new pull request, #16705:
URL: https://github.com/apache/kafka/pull/16705
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### 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
Re: [PR] KAFKA-17109: Reduce log message load for failed locking [kafka]
Posted by "danicafine (via GitHub)" <gi...@apache.org>.
danicafine commented on code in PR #16705:
URL: https://github.com/apache/kafka/pull/16705#discussion_r1720800331
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/TaskManager.java:
##########
@@ -1011,7 +1011,7 @@ private void addTaskToStateUpdater(final Task task) {
} catch (final LockException lockException) {
// The state directory may still be locked by another thread, when the rebalance just happened.
// Retry in the next iteration.
- log.info("Encountered lock exception. Reattempting locking the state in the next iteration.", lockException);
+ log.debug("Encountered lock exception. Reattempting locking the state in the next iteration.");
Review Comment:
Thanks! I've updated.
--
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-17109: Reduce log message load for failed locking [kafka]
Posted by "eduwercamacaro (via GitHub)" <gi...@apache.org>.
eduwercamacaro commented on code in PR #16705:
URL: https://github.com/apache/kafka/pull/16705#discussion_r1714535008
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/TaskManager.java:
##########
@@ -1011,7 +1011,7 @@ private void addTaskToStateUpdater(final Task task) {
} catch (final LockException lockException) {
// The state directory may still be locked by another thread, when the rebalance just happened.
// Retry in the next iteration.
- log.info("Encountered lock exception. Reattempting locking the state in the next iteration.", lockException);
+ log.debug("Encountered lock exception. Reattempting locking the state in the next iteration.");
Review Comment:
I agree.
In this case, I believe the exception message is more useful because it also includes whether it is a standby or active task.
--
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-17109: Reduce log message load for failed locking [kafka]
Posted by "danicafine (via GitHub)" <gi...@apache.org>.
danicafine commented on PR #16705:
URL: https://github.com/apache/kafka/pull/16705#issuecomment-2322520138
@cadonna given that the state updater feature is relatively new, I can see why we'd want to keep it at info-level. Happy to do so if @eduwercamacaro has no objections.
--
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-17109: Reduce log message load for failed locking [kafka]
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #16705:
URL: https://github.com/apache/kafka/pull/16705#discussion_r1713907490
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/TaskManager.java:
##########
@@ -1011,7 +1011,7 @@ private void addTaskToStateUpdater(final Task task) {
} catch (final LockException lockException) {
// The state directory may still be locked by another thread, when the rebalance just happened.
// Retry in the next iteration.
- log.info("Encountered lock exception. Reattempting locking the state in the next iteration.", lockException);
+ log.debug("Encountered lock exception. Reattempting locking the state in the next iteration.");
Review Comment:
If you remove the exception from the log message, we lose the info about to which task this lock exception belongs. So either you add only the message of the `LockException` to the log message or you add the task ID from `task`. I would prefer the former. Something like:
```suggestion
log.debug("Encountered lock exception. Reattempting locking the state in the next iteration. Error message was: {}" lockException.getMessage());
```
--
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-17109: Reduce log message load for failed locking [kafka]
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna merged PR #16705:
URL: https://github.com/apache/kafka/pull/16705
--
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-17109: Reduce log message load for failed locking [kafka]
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on PR #16705:
URL: https://github.com/apache/kafka/pull/16705#issuecomment-2296473623
@danicafine @eduwercamacaro What do you think of letting the log message on info level but without the stacktrace. The state updater feature is quite new and I would prefer to still see at least that the lock exceptions happens without the stack trace during normal processing without the need to switch to debug.
--
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-17109: Reduce log message load for failed locking [kafka]
Posted by "eduwercamacaro (via GitHub)" <gi...@apache.org>.
eduwercamacaro commented on PR #16705:
URL: https://github.com/apache/kafka/pull/16705#issuecomment-2327622257
Sure, no objections here.
--
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-17109: Reduce log message load for failed locking [kafka]
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #16705:
URL: https://github.com/apache/kafka/pull/16705#discussion_r1714877818
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/TaskManager.java:
##########
@@ -1011,7 +1011,7 @@ private void addTaskToStateUpdater(final Task task) {
} catch (final LockException lockException) {
// The state directory may still be locked by another thread, when the rebalance just happened.
// Retry in the next iteration.
- log.info("Encountered lock exception. Reattempting locking the state in the next iteration.", lockException);
+ log.debug("Encountered lock exception. Reattempting locking the state in the next iteration.");
Review Comment:
Good point!
--
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-17109: Reduce log message load for failed locking [kafka]
Posted by "danicafine (via GitHub)" <gi...@apache.org>.
danicafine commented on PR #16705:
URL: https://github.com/apache/kafka/pull/16705#issuecomment-2254203099
Pinging @mjsax @cadonna @eduwercamacaro for 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