artemlivshits commented on code in PR #14629:
URL: https://github.com/apache/kafka/pull/14629#discussion_r1380813788
##########
core/src/main/scala/kafka/server/ReplicaManager.scala:
##########
@@ -864,6 +778,111 @@ class ReplicaManager(val config: KafkaConfig,
}
}
+ /*
+ * Note: This method can be used as a callback in a different request
thread. Ensure that correct RequestLocal
+ * is passed when executing this method. Accessing non-thread-safe data
structures should be avoided if possible.
+ */
+ private def appendEntries(allEntries: Map[TopicPartition, MemoryRecords],
+ internalTopicsAllowed: Boolean,
+ origin: AppendOrigin,
+ requiredAcks: Short,
+ verificationGuards: Map[TopicPartition,
VerificationGuard],
Review Comment:
Hmm, I think it would break the abstraction of the appendRecords being a
fundamentally asynchronous call, i.e. it has the syntax of being asynchronous
(i.e. completion callback is surely called in a different thread) but still a
concrete implementation expects that a specific part of the call is
synchronous, but the code has no indication which one (maybe some day we'd use
async IO and again make it more asynchronous then the code around it would
expect). Like why would the group coordinator need to know about transaction
verification flow which has it's own trickiness with verification guard and may
actually (likely) be different for KIP-890 part2?
I think it would be easier for separation of concerns if the lock is held
only around im-memory state updates and just transition the state into a
"pending state", so that the new updated would be queued or failed (with a
retriable error) until async call is done.
Then we wouldn't have a situation where we have a lock with a syntax that
looks like "lock around in-memory update + call", but with the desired
semantics "[lock] [in-memory update] [some part of call] [unlock] [some other
part of call]" and the place of [unlock] is fairly deep in logic that has no
indication that by doing an unrelated action (making the code as asynchronous
as need to avoid holding threads) it could actually break the desired semantics.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
