Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions core/src/main/scala/kafka/server/AbstractFetcherManager.scala
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,9 @@ abstract class AbstractFetcherManager[T <: AbstractFetcherThread](val name: Stri
tp -> OffsetAndEpoch(brokerAndInitOffset.initOffset, brokerAndInitOffset.currentLeaderEpoch)
}

fetcherThread.addPartitions(initialOffsetAndEpochs)
info(s"Added fetcher to broker ${brokerAndFetcherId.broker} for partitions $initialOffsetAndEpochs")

failedPartitions.removeAll(partitionAndOffsets.keySet)
val addedPartitions = fetcherThread.addPartitions(initialOffsetAndEpochs)
info(s"Added fetcher to broker ${brokerAndFetcherId.broker} for partitions " +
s"${initialOffsetAndEpochs.filterKeys(addedPartitions)}")
}
}
}
Expand Down
7 changes: 5 additions & 2 deletions core/src/main/scala/kafka/server/AbstractFetcherThread.scala
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ abstract class AbstractFetcherThread(name: String,
type EpochData = OffsetsForLeaderEpochRequest.PartitionData

private val partitionStates = new PartitionStates[PartitionFetchState]
private val partitionMapLock = new ReentrantLock
protected val partitionMapLock = new ReentrantLock
private val partitionMapCond = partitionMapLock.newCondition()

private val metricId = ClientIdAndBroker(clientId, sourceBroker.host, sourceBroker.port)
Expand Down Expand Up @@ -420,9 +420,11 @@ abstract class AbstractFetcherThread(name: String,
}


def addPartitions(initialFetchStates: Map[TopicPartition, OffsetAndEpoch]): Unit = {
def addPartitions(initialFetchStates: Map[TopicPartition, OffsetAndEpoch]): Set[TopicPartition] = {
partitionMapLock.lockInterruptibly()
try {
failedPartitions.removeAll(initialFetchStates.keySet)

initialFetchStates.foreach { case (tp, initialFetchState) =>
// We can skip the truncation step iff the leader epoch matches the existing epoch
val currentState = partitionStates.stateValue(tp)
Expand All @@ -437,6 +439,7 @@ abstract class AbstractFetcherThread(name: String,
}

partitionMapCond.signalAll()
initialFetchStates.keySet
} finally partitionMapLock.unlock()
}

Expand Down
21 changes: 19 additions & 2 deletions core/src/main/scala/kafka/server/ReplicaAlterLogDirsThread.scala
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class ReplicaAlterLogDirsThread(name: String,
Request.FutureLocalReplicaId,
request.minBytes,
request.maxBytes,
request.version <= 2,
false,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We always build with the latest fetch version, so this check was unneeded.

request.fetchData.asScala.toSeq,
UnboundedQuota,
processResponseCallback,
Expand All @@ -116,7 +116,11 @@ class ReplicaAlterLogDirsThread(name: String,
throw new IllegalStateException("Offset mismatch for the future replica %s: fetched offset = %d, log end offset = %d.".format(
topicPartition, fetchOffset, futureLog.logEndOffset))

val logAppendInfo = partition.appendRecordsToFollowerOrFutureReplica(records, isFuture = true)
val logAppendInfo = if (records.sizeInBytes() > 0)
partition.appendRecordsToFollowerOrFutureReplica(records, isFuture = true)
else
None

futureLog.updateHighWatermark(partitionData.highWatermark)
futureLog.maybeIncrementLogStartOffset(partitionData.logStartOffset)

Expand All @@ -127,6 +131,19 @@ class ReplicaAlterLogDirsThread(name: String,
logAppendInfo
}

override def addPartitions(initialFetchStates: Map[TopicPartition, OffsetAndEpoch]): Set[TopicPartition] = {
partitionMapLock.lockInterruptibly()
try {
// It is possible that the log dir fetcher completed just before this call, so we
// filter only the partitions which still have a future log dir.
val filteredFetchStates = initialFetchStates.filterKeys(replicaMgr.futureLogExists)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So there is a extra check of future folder here and the check is in the lock so the race condition is resolved.

super.addPartitions(filteredFetchStates)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we skip to create thread if the collection of filtered states is empty?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this will be a no-op in addPartitions, so it's probably harmless

filteredFetchStates.keySet

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems this line is useless.

} finally {
partitionMapLock.unlock()
}
}

override protected def fetchEarliestOffsetFromLeader(topicPartition: TopicPartition, leaderEpoch: Int): Long = {
val partition = replicaMgr.getPartitionOrException(topicPartition, expectLeader = false)
partition.localLogOrException.logStartOffset
Expand Down
4 changes: 4 additions & 0 deletions core/src/main/scala/kafka/server/ReplicaManager.scala
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,10 @@ class ReplicaManager(val config: KafkaConfig,
getPartitionOrException(topicPartition, expectLeader = false).futureLocalLogOrException
}

def futureLogExists(topicPartition: TopicPartition): Boolean = {
getPartitionOrException(topicPartition, expectLeader = false).futureLog.isDefined
}

def localLog(topicPartition: TopicPartition): Option[Log] = {
nonOfflinePartition(topicPartition).flatMap(_.log)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class AbstractFetcherManagerTest {

EasyMock.expect(fetcher.start())
EasyMock.expect(fetcher.addPartitions(Map(tp -> OffsetAndEpoch(fetchOffset, leaderEpoch))))
.andReturn(Set(tp))
EasyMock.expect(fetcher.fetchState(tp))
.andReturn(Some(PartitionFetchState(fetchOffset, None, leaderEpoch, Truncating)))
EasyMock.expect(fetcher.removePartitions(Set(tp)))
Expand Down Expand Up @@ -116,6 +117,7 @@ class AbstractFetcherManagerTest {

EasyMock.expect(fetcher.start())
EasyMock.expect(fetcher.addPartitions(Map(tp -> OffsetAndEpoch(fetchOffset, leaderEpoch))))
.andReturn(Set(tp))
EasyMock.expect(fetcher.isThreadFailed).andReturn(true)
EasyMock.replay(fetcher)

Expand Down
Loading