divijvaidya commented on code in PR #14629:
URL: https://github.com/apache/kafka/pull/14629#discussion_r1372844362
##########
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:
1. Are we sure that the Maps being passed here are not concurrently modified
by two threads? Asking because they are not thread safe (they aren't concurrent
hash maps). I haven't looked in details about the params passed here so please
feel free to say that you have already verified this.
2. How can we prevent future bugs where someone mutates one of these
parameters in perhaps other parts of the code such as Kafka APIs where
appendEntries is called from without knowing that all these data structures are
supposed to be thread safe?
One option to prevent future bugs could be to pass a deep copy of these
objects to the callback (instead of a copy of the reference). Another thing we
can do is to restrict the number of parameters required by the callback.
Having said that, have we already considered not having this as a callback
and instead appending this synchronously? I understand that it has drawbacks
since it is in critical produce path but maybe that would be acceptable given
the complexity of ensuring thread safety here?
--
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]
