You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/03/29 22:42:14 UTC
[GitHub] [kafka] kowshik opened a new pull request #10430: KAFKA-12575: Eliminate Log.isLogDirOffline boolean attribute
kowshik opened a new pull request #10430:
URL: https://github.com/apache/kafka/pull/10430
**This PR is a precursor to the recovery logic refactor work (KAFKA-12553).**
I have made a change to eliminate `Log. isLogDirOffline` attribute. This attribute was added in https://github.com/apache/kafka/pull/9676. But it is redundant and can be eliminated in favor of looking up `LogDirFailureChannel` to check if the `logDir` is offline. The performance implication of such a hash map lookup inside `LogDirFailureChannel` should be low/none because `ConcurrentHashMap` reads are usually lock free.
**Tests:**
Relying on existing unit/integration tests.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao merged pull request #10430: KAFKA-12575: Eliminate Log.isLogDirOffline boolean attribute
Posted by GitBox <gi...@apache.org>.
junrao merged pull request #10430:
URL: https://github.com/apache/kafka/pull/10430
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on pull request #10430: KAFKA-12575: Eliminate Log.isLogDirOffline boolean attribute
Posted by GitBox <gi...@apache.org>.
kowshik commented on pull request #10430:
URL: https://github.com/apache/kafka/pull/10430#issuecomment-809766262
cc @dhruvilshah3 @junrao 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on pull request #10430: KAFKA-12575: Eliminate Log.isLogDirOffline boolean attribute
Posted by GitBox <gi...@apache.org>.
kowshik commented on pull request #10430:
URL: https://github.com/apache/kafka/pull/10430#issuecomment-809805502
cc @hachikuji
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on a change in pull request #10430: KAFKA-12575: Eliminate Log.isLogDirOffline boolean attribute
Posted by GitBox <gi...@apache.org>.
kowshik commented on a change in pull request #10430:
URL: https://github.com/apache/kafka/pull/10430#discussion_r603682389
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -2454,12 +2444,13 @@ class Log(@volatile private var _dir: File,
private[log] def addSegment(segment: LogSegment): LogSegment = this.segments.put(segment.baseOffset, segment)
private def maybeHandleIOException[T](msg: => String)(fun: => T): T = {
+ if (logDirFailureChannel.hasOfflineLogDir(parentDir)) {
+ throw new KafkaStorageException(s"The log dir $parentDir is offline due to a previous IO exception.")
+ }
try {
- checkForLogDirFailure()
fun
} catch {
case e: IOException =>
- logDirOffline = true
logDirFailureChannel.maybeAddOfflineLogDir(dir.getParent, msg, e)
Review comment:
Done, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on pull request #10430: KAFKA-12575: Eliminate Log.isLogDirOffline boolean attribute
Posted by GitBox <gi...@apache.org>.
kowshik commented on pull request #10430:
URL: https://github.com/apache/kafka/pull/10430#issuecomment-809796561
@dhruvilshah3 Thanks for the review! I've addressed your comments in 2d11384.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on pull request #10430: KAFKA-12575: Eliminate Log.isLogDirOffline boolean attribute
Posted by GitBox <gi...@apache.org>.
kowshik commented on pull request #10430:
URL: https://github.com/apache/kafka/pull/10430#issuecomment-811278507
@junrao @dhruvilshah3 @hachikuji the Jenkins tests have finished and the failed ones are not related to this PR.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dhruvilshah3 commented on a change in pull request #10430: KAFKA-12575: Eliminate Log.isLogDirOffline boolean attribute
Posted by GitBox <gi...@apache.org>.
dhruvilshah3 commented on a change in pull request #10430:
URL: https://github.com/apache/kafka/pull/10430#discussion_r603666511
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -2454,12 +2444,13 @@ class Log(@volatile private var _dir: File,
private[log] def addSegment(segment: LogSegment): LogSegment = this.segments.put(segment.baseOffset, segment)
private def maybeHandleIOException[T](msg: => String)(fun: => T): T = {
+ if (logDirFailureChannel.hasOfflineLogDir(parentDir)) {
+ throw new KafkaStorageException(s"The log dir $parentDir is offline due to a previous IO exception.")
+ }
try {
- checkForLogDirFailure()
fun
} catch {
case e: IOException =>
- logDirOffline = true
logDirFailureChannel.maybeAddOfflineLogDir(dir.getParent, msg, e)
Review comment:
nit: could we use `parentDir` here instead of `dir.getParent`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org