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

[BEAM-2257] Fixes bug with Kafka Producer needing a key serializer set in the case of value only PCollection #3969

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@nerdynick
Contributor

nerdynick commented Oct 10, 2017

…auto add the VoidSerializer for the key.serializer config for kafka producer

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@aaltay

This comment has been minimized.

Show comment
Hide comment
@aaltay

aaltay Oct 20, 2017

Contributor
Contributor

aaltay commented Oct 20, 2017

@rangadi

This comment has been minimized.

Show comment
Hide comment
@rangadi

rangadi Oct 20, 2017

Contributor

Thanks @nerdynick. Could you fix JavaDoc as well as pointed out in BEAM-2257?

Contributor

rangadi commented Oct 20, 2017

Thanks @nerdynick. Could you fix JavaDoc as well as pointed out in BEAM-2257?

@nerdynick

This comment has been minimized.

Show comment
Hide comment
@nerdynick

nerdynick Oct 20, 2017

Contributor

What JavaDoc are you referring to as needing to be updated? @rangadi

Contributor

nerdynick commented Oct 20, 2017

What JavaDoc are you referring to as needing to be updated? @rangadi

@rangadi

This comment has been minimized.

Show comment
Hide comment
@rangadi

rangadi Oct 20, 2017

Contributor

At line 223. The argument for withValueSerializer() should be StringSerializer.class.

Contributor

rangadi commented Oct 20, 2017

At line 223. The argument for withValueSerializer() should be StringSerializer.class.

@nerdynick

This comment has been minimized.

Show comment
Hide comment
@nerdynick

nerdynick Oct 20, 2017

Contributor

Fixed. Thought you where referring to documentation around this change. Not the sub-issue that was reported.

Contributor

nerdynick commented Oct 20, 2017

Fixed. Thought you where referring to documentation around this change. Not the sub-issue that was reported.

@rangadi

This comment has been minimized.

Show comment
Hide comment
@rangadi

rangadi Oct 20, 2017

Contributor

LGTM. Thanks @nerdynick.
@aaltay : Please merge when you get a chance.

Contributor

rangadi commented Oct 20, 2017

LGTM. Thanks @nerdynick.
@aaltay : Please merge when you get a chance.

@aaltay

This comment has been minimized.

Show comment
Hide comment
@aaltay

aaltay Oct 20, 2017

Contributor

Thank you. I can merge after java tests pass.

Contributor

aaltay commented Oct 20, 2017

Thank you. I can merge after java tests pass.

@aaltay

This comment has been minimized.

Show comment
Hide comment
@aaltay

aaltay Oct 20, 2017

Contributor

Also there is a conflict, @nerdynick could you rebase this please?

Contributor

aaltay commented Oct 20, 2017

Also there is a conflict, @nerdynick could you rebase this please?

@nerdynick

This comment has been minimized.

Show comment
Hide comment
@nerdynick

nerdynick Oct 20, 2017

Contributor

Merged upstream and rebased @aaltay

Contributor

nerdynick commented Oct 20, 2017

Merged upstream and rebased @aaltay

@rangadi

This comment has been minimized.

Show comment
Hide comment
@rangadi

rangadi Oct 20, 2017

Contributor

@aaltay, please wait for couple more small fixes.

Contributor

rangadi commented Oct 20, 2017

@aaltay, please wait for couple more small fixes.

@nerdynick

This comment has been minimized.

Show comment
Hide comment
@nerdynick

nerdynick Oct 21, 2017

Contributor

@rangadi It looks like the move to the StringSerializer messed up the generics casting. Which was something I forgot I battled to get the VoidSerializer to work. I might have another approach that would keep the StringSerializer but remove the need for the casting.

Contributor

nerdynick commented Oct 21, 2017

@rangadi It looks like the move to the StringSerializer messed up the generics casting. Which was something I forgot I battled to get the VoidSerializer to work. I might have another approach that would keep the StringSerializer but remove the need for the casting.

@rangadi

This comment has been minimized.

Show comment
Hide comment
@rangadi

rangadi Oct 23, 2017

Contributor

I don't think you can void casting. Please run mvn test -pl sdks/java/io/kafka to verify it passes compilation and check-style. A simple fix would be to remove type parameter in cast as mentioned earlier (.setKeySerializer((Class) StringSerializer.class)).

Contributor

rangadi commented Oct 23, 2017

I don't think you can void casting. Please run mvn test -pl sdks/java/io/kafka to verify it passes compilation and check-style. A simple fix would be to remove type parameter in cast as mentioned earlier (.setKeySerializer((Class) StringSerializer.class)).

@nerdynick

This comment has been minimized.

Show comment
Hide comment
@nerdynick

nerdynick Oct 23, 2017

Contributor

Doing the cast to Class results in a Rawtypes warning. Was trying to avoid that as it just seems messy.

Contributor

nerdynick commented Oct 23, 2017

Doing the cast to Class results in a Rawtypes warning. Was trying to avoid that as it just seems messy.

@nerdynick

This comment has been minimized.

Show comment
Hide comment
@nerdynick

nerdynick Oct 24, 2017

Contributor

The change is made. Running the command you gave me fails however. It complains about the AutoValue classes not getting built. However doing the same/similar change to a pipeline I have works.

Contributor

nerdynick commented Oct 24, 2017

The change is made. Running the command you gave me fails however. It complains about the AutoValue classes not getting built. However doing the same/similar change to a pipeline I have works.

@rangadi

This comment has been minimized.

Show comment
Hide comment
@rangadi

rangadi Oct 24, 2017

Contributor

Thanks. Please remove the stale the comment. We can merge after that.

Contributor

rangadi commented Oct 24, 2017

Thanks. Please remove the stale the comment. We can merge after that.

@nerdynick

This comment has been minimized.

Show comment
Hide comment
@nerdynick

nerdynick Oct 24, 2017

Contributor

Removed.

Contributor

nerdynick commented Oct 24, 2017

Removed.

@rangadi

This comment has been minimized.

Show comment
Hide comment
@rangadi

rangadi Oct 24, 2017

Contributor

+1. LGTM.
@aaltay or @chamikaramj : please merge when you get a chance.

Contributor

rangadi commented Oct 24, 2017

+1. LGTM.
@aaltay or @chamikaramj : please merge when you get a chance.

@chamikaramj

This comment has been minimized.

Show comment
Hide comment
@chamikaramj

chamikaramj Oct 25, 2017

Contributor

Run Java Precommit

Contributor

chamikaramj commented Oct 25, 2017

Run Java Precommit

@rangadi rangadi referenced this pull request Oct 26, 2017

Closed

Fix Javadoc for writing to kafka with KafkaIO #4030

0 of 6 tasks complete
@nerdynick

This comment has been minimized.

Show comment
Hide comment
@nerdynick

nerdynick Oct 26, 2017

Contributor

@chamikaramj What do you mean by this

Contributor

nerdynick commented Oct 26, 2017

@chamikaramj What do you mean by this

@rangadi

This comment has been minimized.

Show comment
Hide comment
@rangadi

rangadi Oct 26, 2017

Contributor

@nerdynick it is just a way to trigger Jenkins build. You can ignore it for now.

Contributor

rangadi commented Oct 26, 2017

@nerdynick it is just a way to trigger Jenkins build. You can ignore it for now.

@chamikaramj

This comment has been minimized.

Show comment
Hide comment
@chamikaramj

chamikaramj Oct 26, 2017

Contributor

Yeah, seems like Jenkins runs are pretty unstable currently. Will merge when Jenkins pass. "Run Java Precommit" triggers Java pre-commit Jenkins test suite.

Contributor

chamikaramj commented Oct 26, 2017

Yeah, seems like Jenkins runs are pretty unstable currently. Will merge when Jenkins pass. "Run Java Precommit" triggers Java pre-commit Jenkins test suite.

@chamikaramj

This comment has been minimized.

Show comment
Hide comment
@chamikaramj

chamikaramj Oct 28, 2017

Contributor

Run Java Precommit

Contributor

chamikaramj commented Oct 28, 2017

Run Java Precommit

@rangadi

This comment has been minimized.

Show comment
Hide comment
@rangadi

rangadi Oct 31, 2017

Contributor

Run Java Precommit.
@chamikaramj, please merge this if there isn't much chance that jenkins would pass.

Contributor

rangadi commented Oct 31, 2017

Run Java Precommit.
@chamikaramj, please merge this if there isn't much chance that jenkins would pass.

@asfgit asfgit closed this in 54bc58b Oct 31, 2017

@nerdynick

This comment has been minimized.

Show comment
Hide comment
@nerdynick

nerdynick Nov 7, 2017

Contributor

@chamikaramj or @rangadi Has this been merged? Is there a plan when it'll be packaged for release.

Contributor

nerdynick commented Nov 7, 2017

@chamikaramj or @rangadi Has this been merged? Is there a plan when it'll be packaged for release.

@chamikaramj

This comment has been minimized.

Show comment
Hide comment
@chamikaramj

chamikaramj Nov 7, 2017

Contributor

Yeah, this was merged to HEAD. It will be included in the next release (2.3.0).

Contributor

chamikaramj commented Nov 7, 2017

Yeah, this was merged to HEAD. It will be included in the next release (2.3.0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment