BEAM-307: Upgrade/Test to Kafka 0.10#2069
BEAM-307: Upgrade/Test to Kafka 0.10#2069mingmxu wants to merge 5 commits intoapache:masterfrom mingmxu:master
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
pull latest code
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
rangadi
left a comment
rangadi
left a comment
There was a problem hiding this comment.
Thanks @xumingmin.
Couple of comments. LGTM with the those fixed.
sdks/java/io/kafka/pom.xml
Outdated
| <description>Library to read Kafka topics.</description> | ||
|
|
||
| <properties> | ||
| <kafka.clients.version>0.10.1.1</kafka.clients.version> |
There was a problem hiding this comment.
Can we keep the default to 0.9.0.1? Looks like in Kafka, older clients can connect to newer Kafka servers, but not the other way around.
There was a problem hiding this comment.
my bad, it should be kept as 0.9.0.1
| * Kafka version 0.9 and 0.10 are supported. If you need a specific version of Kafka | ||
| * client(e.g. 0.9 for 0.9 servers, or 0.10 for security features), specify explicit | ||
| * kafka-client dependency. | ||
| * kafka-client dependency with Maven property ${kafka.clients.version}. |
There was a problem hiding this comment.
I think we should remove this update. Uses are not building beam or beam modules by themselves, rather are just using published mvn artifacts. This property is not irrelevant to them.
| @Override | ||
| //remove keyword '@Override' here, it can work with Kafka client 0.9 and 0.10 as: | ||
| //1. SpEL can find this function, either input is List or Collection; | ||
| //2. List extends Collection, so super.assign() could find either assign(List) |
There was a problem hiding this comment.
Makes sense. Thanks for the comment.
|
Refer to this link for build results (access rights to CI server needed): |
|
👍 LGTM. Thanks Xu. |
davorbonaci
left a comment
davorbonaci
left a comment
There was a problem hiding this comment.
LGTM. Merging.
The next step would be to automate execution of this in the CI system. @xumingmin, let me know if you are interested in taking this on.
|
@davorbonaci , more details about 'taking this on'? |
|
This PR is making it possible to run against multiple versions of Kafka with a single flag. The CI infrastructure is, however, using one, specific version. So, we won't know if/when something breaks here. It would be great to automate testing against multiple versions. This task would be non-trivial. It can be a new module that changes the dependency. Or, it can be another test invocation in the same module that overrides the version for the second invocation. (Other options available too.) All this may or may not involve spinning multiple-versions of Kafka clusters via, say, Kubernates. Overall, this is a complex infrastructure work -- it is totally fine if this is not something you are personally interested in. We'll get there eventually ;-) |
|
Simplest is extra test invocation in one one of the builds (ci, jenkins, or nightly): e.g. we could add this to |
|
Indeed -- but, that approach won't scale for our project-wide needs, and goes against many of the goals we have set out for our engineering system (e.g., easy for contributors to run the tests, no complicated commands to reproduce what the CI is running, interop with plugins, etc.). So, I'd like to go beyond this. |
|
I'll look into this after timestamp support. |
5 participants
Run KafkaIOTest.java with
mvn test -Dkafka.clients.version=0.10.1.1, ormvn test -Dkafka.clients.version=0.9.0.1for either Kafka client version.it can work with Kafka client 0.9 and 0.10 as: