-
Notifications
You must be signed in to change notification settings - Fork 148
APEXMALHAR-2497 APEXMALHAR-2162 1) Refactor the Exactly Once output operator. 2) Refactor and fix the issues of unit tests. #642
base: master
Are you sure you want to change the base?
Conversation
|
||
public abstract class AbstractEmbeddedKafka | ||
{ | ||
private static final String[] KAFKA_PATH = new String[]{"/tmp/kafka-test1","/tmp/kafka-test2"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should not be such hard coded tmp directories.
import kafka.utils.ZKStringSerializer$; | ||
import kafka.utils.ZkUtils; | ||
|
||
public abstract class AbstractEmbeddedKafka |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed, why not use kafka-unit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tweise kafka-unit has a limit on number of connections. It's a hard limit and doesn't have an option to increase connections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which test is affected by that? There are several other embedded Kafka examples to look at, here is another one: https://github.com/confluentinc/examples/blob/3.2.x/kafka-streams/src/test/java/io/confluent/examples/streams/kafka/EmbeddedSingleNodeKafkaCluster.java
} | ||
} | ||
|
||
public static AbstractEmbeddedKafka kafkaServer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this be
Rule
public KafkaUnitRule kafkaUnitRule = new KafkaUnitRule(zkPort, brokerPort);
and if multiple servers are needed create multiple rules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
Please submit separate PRs for the two JIRAs. That will simplify review and is the recommended approach in general. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous review comments.
…perator. 2) Refactor and fix the issues of unit tests.
54565e0
to
15d2903
Compare
@tushargosavi @tweise Please review and merge.