zhangyue19921010 commented on a change in pull request #10551:
URL: https://github.com/apache/druid/pull/10551#discussion_r535297518
##########
File path:
extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaConsumerConfigs.java
##########
@@ -31,14 +31,14 @@
public class KafkaConsumerConfigs
{
- public static Map<String, Object> getConsumerProperties()
+ public static Map<String, Object> getConsumerProperties(Map<String, Object>
customerConsumerProperties)
Review comment:
Hi @himanshug Thanks for your review!
I agree with `it would likely be simpler to leave this alone, remove the
line props.put("isolation.level".. altogether`. But I think maybe it is not
very appropriate to modify
`https://github.com/apache/druid/blob/master/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfig.java#L78`
I have tried to do changes in `KafkaSupervisorIOConfig.java` as you said
(https://github.com/apache/druid/pull/10551/commits/cc59d8bbe0175faa269a4bdc06a24f13f6dd220c#)
```
this.consumerProperties = Preconditions.checkNotNull(consumerProperties,
"consumerProperties");
consumerProperties.putIfAbsent("isolation.level", "read_committed");
Preconditions.checkNotNull(
```
and functional testing is fine while UT is failed at
https://github.com/apache/druid/blob/ac6d703814ccb5b258c586b63e0bc33d669e0f57/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/KafkaIOConfigTest.java#L363
and
https://github.com/apache/druid/blob/ac6d703814ccb5b258c586b63e0bc33d669e0f57/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/KafkaIOConfigTest.java#L332
These failure means changes in KafkaSupervisorIOConfig may cause
compatibility issues when deserialize from old IoConfig to new IoConfig or
deserialize from new IoConfig to old IoConfig. In other words, we need to
ensure that the deserialize between new IoConfig and old IoConfig is completely
consistent and smooth.
So I switched to a safer way in the latest commit.
All the changes is tested on Dev cluster.
PTAL :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
