Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1623#issuecomment-185307346
I have some comments, here is a first batch:
The `ZooKeeperStringSerializer` seems to be only used in tests, but adds a
hard compile-scope dependency to `org.I0Itec:ZkClient`. Moving this to test
scope gets rid of this.
There are some old functions that seem to have no more use:
- KafkaTopicPartitionLeader.replaceIgnoringLeader()
- KafkaTopicPartition.isContained()
In the `OffsetHandler` / `ZookeeperOffsetHandler`, the `getOffsets(...)`
methods can probably get rid of the `Fetcher` (since it does not position the
fetcher any more.
There is a lot of option parsing with no checks and proper exceptions. If
someone puts in a wrong value, all they see is an exception in
`Integer.valueOf()`. In most other places of the code, there is a lot of effort
to give good messages on parse errors.
Note: Given all the discussion about dependency clashes / shading and
resulting issues, I would like to reduce dependencies as much as possible. The
Kafka code adds the Guava dependency simply for the `checkNotNull` method. I
would simpyl use `java.util.Object.requiteNotNull` instead and get rid of the
dependency.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
