artemlivshits commented on code in PR #14629:
URL: https://github.com/apache/kafka/pull/14629#discussion_r1375329122
##########
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:
That's correct -- we don't create new concurrent access, i.e. this callback
is not called on multiple threads at the same time, all we do here is
potentially executing this callback on a different thread. Actually I think
"Accessing non-thread-safe data structures should be avoided if possible"
comment is misleading here, the only thing that we have to avoid is capturing
thread local (or effectively thread local) objects in the callback. Other
objects can be as thread safe as they would be if this method was called
directly on this thread (i.e. if an object was rooted on the call stack it
doesn't have to be thread safe because it's not going to be used concurrently
from multiple threads).
BTW, we have plenty of cases in Kafka (both client and server) where we pass
a callback to continue execution of a single-threaded logical task after an RPC
call is complete (most likely on a different thread), so I don't think we're
dealing with unique pattern here. The only thing that is unique (and tricky)
here is the RequestLocal that disguises as a context of a logical task
(arguments are generally are logical task context), but it is actually a
concrete physical thread context that would be different (or missing
completely, say if we executed this callback on the network thread) if we
continued execution of this logical task on a different thread.
I've already pointed out elsewhere that (in a future change) we should
probably remove the RequestLocal from the argument list and make it a
thread-local (which it effectively is) and just use it directly where it's
needed.
--
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]
