Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-3067] Use curator for committing offsets to ZK from Kafka #1451

Closed
wants to merge 2 commits into from

Conversation

rmetzger
Copy link
Contributor

@aljoscha reported issues with our latest FlinkKafkaConnector: It was still throwing NPE.
Therefore, I decided to abandon the zkclient and use Apache Curator instead.

@@ -632,6 +632,7 @@ public void run() {
// ------------ commit current offsets ----------------

// create copy of current offsets
//noinspection unchecked
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this this is only suppressing IntellIJ warnings or is this meant to be a replacement for the @SuppressWarnings annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its only supressing IntelliJ warnings I think. I don't know if we have any policy regarding this. The annotation will also suppress compiler warnings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove the line after the annotation. I don't think we have a policy about this, but I've seen it in many places.

@aljoscha
Copy link
Contributor

I tested the changes on the cluster with a job where I previously saw failures in zkClient. Now it seems to work. So I would give my :+1. Others would probably look at the implementation but that seems to be going on already.


private final CuratorFramework curatorClient;


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty line

@uce
Copy link
Contributor

uce commented Dec 16, 2015

With Aljoscha's test +1 to merge (minus two trivial comments).

@StephanEwen
Copy link
Contributor

Good fix, merging this!

StephanEwen pushed a commit to StephanEwen/flink that referenced this pull request Dec 27, 2015
@asfgit asfgit closed this in 4f8c5e8 Dec 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants