-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-14544 The "is-future" should be removed from metrics tags after future log becomes current log #12979
Conversation
Thanks for the patch. I'll take a look this week. |
file a jira as this is related to true bug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chia7712 , thanks for the PR. Left one comment.
destLog.removeLogMetrics() | ||
destLog.renameDir(UnifiedLog.logDirName(topicPartition), true) | ||
destLog.updateHighWatermark(sourceLog.highWatermark) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should remove log metrics after destLog got renamed? I'm thinking if we got stuck for some time during renameDir
and it'll be helpful to have metrics reporting it. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'll be helpful to have metrics reporting it
make sense. will copy that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the fix!
the new test is unstable. Will fix it later. |
@showuon Could you take a look again? the latest commit includes extra changes to address your comment. |
private[log] def removeLogMetrics(): Unit = { | ||
removeMetric(LogMetricNames.NumLogSegments, tags) | ||
removeMetric(LogMetricNames.LogStartOffset, tags) | ||
removeMetric(LogMetricNames.LogEndOffset, tags) | ||
removeMetric(LogMetricNames.Size, tags) | ||
metricNames.foreach { | ||
case (name, tags) => removeMetric(name, tags) | ||
} | ||
metricNames = Map.empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why we need this change to remove metrics, instead of the original one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it because when we tried to removeMetrics, the tags might become "non-future"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it because when we tried to removeMetrics, the tags might become "non-future"?
yep. The tags DOES NOT include "future" after the log dir get renamed. Hence, we have to keep previous metrics name in order to delete correct metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks.
… future log becomes current log (apache#12979) Reviewers: Luke Chen <showuon@gmail.com>
we don't remove "is-future=true" tag from future log after the future log becomes "current" log. It causes two potential issues:
https://issues.apache.org/jira/browse/KAFKA-14544
Committer Checklist (excluded from commit message)