jolshan commented on code in PR #14629:
URL: https://github.com/apache/kafka/pull/14629#discussion_r1380899701
##########
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:
> I was saying that appendEntries queues up a log record and we could use it
there.
>
> I think Artem was not necessarily disagreeing with that idea but rather
that a lock should be used at all. My understanding of his comment was that we
should move to a model where changes are pending and those state transitions
block further work.
>
> I think we also discussed that the ordering of the log is not the
important part but rather committing stale data. It was unclear to me from
Jason's comments if this validation is fully necessary due to how we can
sometimes write stale data to a transaction when a rebalance happens after
records are written but before the transaction is committed.
>
> Basically there are two questions I see:
>
> 1. Can we make this code more async friendly to avoid locking? (And does
the new group coordinator do this?)
> 2. Do we really need the lock if we can write inconsistent data outside
the typical locking mechanisms and we handle it correctly in those cases?
##########
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:
> I was saying that appendEntries queues up a log record and we could use it
there.
>
> I think Artem was not necessarily disagreeing with that idea but rather
that a lock should be used at all. My understanding of his comment was that we
should move to a model where changes are pending and those state transitions
block further work.
>
> I think we also discussed that the ordering of the log is not the
important part but rather committing stale data. It was unclear to me from
Jason's comments if this validation is fully necessary due to how we can
sometimes write stale data to a transaction when a rebalance happens after
records are written but before the transaction is committed.
>
> Basically there are two questions I see:
>
> 1. Can we make this code more async friendly to avoid locking? (And does
the new group coordinator do this?)
> 2. Do we really need the lock if we can write inconsistent data outside
the typical locking mechanisms and we handle it correctly in those cases?
--
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]
