Skip to content

KAFKA-10141: Add more detail to log segment delete messages#8850

Merged
hachikuji merged 6 commits intoapache:trunkfrom
skaundinya15:KAFKA-10141
Jun 19, 2020
Merged

KAFKA-10141: Add more detail to log segment delete messages#8850
hachikuji merged 6 commits intoapache:trunkfrom
skaundinya15:KAFKA-10141

Conversation

@skaundinya15
Copy link
Copy Markdown
Contributor

@skaundinya15 skaundinya15 commented Jun 11, 2020

As specified in https://issues.apache.org/jira/browse/KAFKA-10141, it would be helpful to include as much information as possible when deleting log segments. This patch introduces log messages that give more specific details as to why the log segment was deleted and the specific metadata regarding that log segment.

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)

hachikuji
hachikuji reviewed Jun 11, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick patch. Left some small suggestions.

Comment thread core/src/main/scala/kafka/log/Log.scala Outdated
Comment thread core/src/main/scala/kafka/log/Log.scala Outdated
Comment thread core/src/main/scala/kafka/log/Log.scala Outdated
@hachikuji
Copy link
Copy Markdown
Contributor

hachikuji commented Jun 11, 2020

ok to test

@skaundinya15 skaundinya15 requested a review from hachikuji June 11, 2020 07:05
@skaundinya15
Copy link
Copy Markdown
Contributor Author

skaundinya15 commented Jun 11, 2020

Thanks for the review @hachikuji, just updated the PR addressing your comments. The PR is ready for a review whenever you are.

hachikuji
hachikuji reviewed Jun 11, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, just a couple more small comments.

Comment thread core/src/main/scala/kafka/log/Log.scala Outdated
Comment thread core/src/main/scala/kafka/log/Log.scala Outdated
Comment thread core/src/main/scala/kafka/log/Log.scala Outdated
@skaundinya15
Copy link
Copy Markdown
Contributor Author

skaundinya15 commented Jun 12, 2020

Thanks for the reviews @hachikuji. I updated the PR per your suggestions and left some follow up comments for clarification. Whenever you're ready, the PR is ready for another review.

@skaundinya15 skaundinya15 requested a review from hachikuji June 15, 2020 16:57
hachikuji
hachikuji reviewed Jun 17, 2020
View reviewed changes
Comment thread core/src/main/scala/kafka/log/Log.scala Outdated
Comment thread core/src/main/scala/kafka/log/Log.scala Outdated
Comment thread core/src/main/scala/kafka/log/Log.scala Outdated
Comment thread core/src/main/scala/kafka/log/Log.scala Outdated
@skaundinya15 skaundinya15 requested a review from hachikuji June 17, 2020 18:50
@skaundinya15
Copy link
Copy Markdown
Contributor Author

skaundinya15 commented Jun 17, 2020

Thanks for the comments @hachikuji. I've addressed all of your feedback, ready for another look whenever you are.

hachikuji
hachikuji approved these changes Jun 18, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall. Just one more nitpick.

Comment thread core/src/main/scala/kafka/log/Log.scala Outdated
@skaundinya15
Copy link
Copy Markdown
Contributor Author

skaundinya15 commented Jun 18, 2020

Thanks @hachikuji, addressed the comment.

@hachikuji
Copy link
Copy Markdown
Contributor

hachikuji commented Jun 18, 2020

retest this please

2 similar comments
@hachikuji
Copy link
Copy Markdown
Contributor

hachikuji commented Jun 18, 2020

retest this please

@hachikuji
Copy link
Copy Markdown
Contributor

hachikuji commented Jun 18, 2020

retest this please

@hachikuji
Copy link
Copy Markdown
Contributor

hachikuji commented Jun 19, 2020

retest this please

2 similar comments
@hachikuji
Copy link
Copy Markdown
Contributor

hachikuji commented Jun 19, 2020

retest this please

@hachikuji
Copy link
Copy Markdown
Contributor

hachikuji commented Jun 19, 2020

retest this please

@hachikuji hachikuji merged commit 1e5fa9e into apache:trunk Jun 19, 2020
ijuma
ijuma reviewed Jun 19, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@ijuma ijuma left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very useful to have the additional information in the logs. It's a bit weird to log these messages when the predicate is executed though (predicates should generally be side effect free). I wonder if this would be more idiomatic if we replaced the predicate with a function that returned Option[String]. None means it's not deletable and Some means it's deletable and the String is the reason. Thoughts?

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 19, 2020

I guess a downside is that we would always generate the string even if logging was only at the warn level.

@skaundinya15 skaundinya15 deleted the KAFKA-10141 branch June 19, 2020 16:33
Kvicii pushed a commit to Kvicii/kafka that referenced this pull request Jun 21, 2020
Merge branch 'trunk' of github.com:apache/kafka into trunk-read
137d186 
* 'trunk' of github.com:apache/kafka:
  KAFKA-10168: fix StreamsConfig parameter name variable (apache#8865)
  MINOR: code cleanup for inconsistent naming (apache#8871)
  KAFKA-10138: Prefer --bootstrap-server for reassign_partitions command in ducktape tests (apache#8898)
  KAFKA-10185: Restoration info logging (apache#8896)
  KAFKA-9891: add integration tests for EOS and StandbyTask (apache#8890)
  MINOR: Reduce build time by gating test coverage plugins behind a flag (apache#8899)
  KAFKA-10141; Add more detail to log segment delete messages (apache#8850)
  KAFKA-10113; Specify fetch offsets correctly in `LogTruncationException` (apache#8822)
  KAFKA-10167: use the admin client to read end-offset (apache#8876)
  MINOR: Upgrade ducktape to 0.7.8 (apache#8879)
  KAFKA-10123; Fix incorrect value for AWAIT_RESET#hasPosition (apache#8841)
  KAFKA-9896: fix flaky StandbyTaskEOSIntegrationTest (apache#8883)
  MINOR: clean up unused checkstyle suppressions for Streams (apache#8861)
  MINOR: reuse toConfigObject(Map) to generate Config (apache#8889)
  MINOR: Upgrade jetty to 9.4.27.v20200227 and jersey to 2.31 (apache#8859)
  MINOR: Fix flaky HighAvailabilityTaskAssignorIntegrationTest (apache#8884)
  KAFKA-10147 MockAdminClient#describeConfigs(Collection<ConfigResource>) is unable to handle broker resource (apache#8853)
  KAFKA-10165: Remove Percentiles from e2e metrics (apache#8882)

# Conflicts:
#	core/src/main/scala/kafka/log/Log.scala
@dhruvilshah3
Copy link
Copy Markdown
Contributor

dhruvilshah3 commented Aug 1, 2020

I attempted to improve the logging further. This also removes the side effect of logging as part of evaluating the predicate. #9110

@dhruvilshah3 dhruvilshah3 mentioned this pull request Aug 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ijuma ijuma ijuma left review comments

@hachikuji hachikuji hachikuji approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@skaundinya15 @hachikuji @ijuma @dhruvilshah3