gaurav-narula commented on code in PR #15885:
URL: https://github.com/apache/kafka/pull/15885#discussion_r1598102664
##########
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/ConsumerTask.java:
##########
@@ -153,6 +173,7 @@ public void run() {
private void processConsumerRecord(ConsumerRecord<byte[], byte[]> record) {
final RemoteLogMetadata remoteLogMetadata =
serde.deserialize(record.value());
+ onConsume.accept(remoteLogMetadata);
Review Comment:
> Nit: Why are we using the supplier pattern instead of adding a setter to
TopicBasedRemoteLogMetadataManager and marking it as visibleForTesting?
IIUC, you're alluding to something similar we do for
`remoteLogMetadataTopicPartitioner` at
https://github.com/apache/kafka/blob/8a9dd2beda90f04180e71b406657d9da388d359e/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerHarness.java#L108.
That can work, but I feel it's very easy to introduce a race inadvertently
since `TopicBasedRemoteLogMetadataManager::configure` spawns a thread
(https://github.com/apache/kafka/blob/8a9dd2beda90f04180e71b406657d9da388d359e/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManager.java#L368).
In fact, `remoteLogMetadataTopicPartitioner` is prone to a race, where if the
test thread yields before line 109 is executed, the `ProducerManager` and
`ConsumerManager` instances can get instantiated with incorrect
`remoteLogMetdataTopicPartitioner` instance
(https://github.com/apache/kafka/blob/8a9dd2beda90f04180e71b406657d9da388d359e/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManager.java#L421).
We can avoid it by invoking the setter before calling
`TopicBasedRemoteLogMetadataManager::configure` but I feel it's easier to
enforce it by using a `Supplier` instead. Either way, I feel this race should
be fixed as well now :)
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
