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

[BEAM-9364] Refactor KafkaIO to use DeserializerProviders #10947

Merged

Conversation

iemejia
Copy link
Member

@iemejia iemejia commented Feb 24, 2020

This builds on your previous PR with some API improvements. Since this adjusts the public API that will be out as part of 2.20.0 is probably better if we include it as part of 2.20.0 too.

R: @aromanenko-dev

@aromanenko-dev
Copy link
Contributor

Run Java KafkaIO Performance Test

@iemejia
Copy link
Member Author

iemejia commented Feb 24, 2020

Weird it did not fail, but it is referring an incorrect (older) run. See here for the actual run
https://builds.apache.org/job/beam_PerformanceTests_Kafka_IO/452/

Copy link
Contributor

@aromanenko-dev aromanenko-dev left a comment

Choose a reason for hiding this comment

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

Thanks, @iemejia! I believe this is a great improvement of code organisation and potential extendability in the future. It looks fine in general for me, I left several minor notes, PTAL.

@rangadi perhaps, it can be interesting for you too (if you have some time).

@iemejia iemejia force-pushed the BEAM-9364-kafkaio-deserializerproviders branch from 8b0a248 to 00cf821 Compare February 25, 2020 09:42
@iemejia
Copy link
Member Author

iemejia commented Feb 25, 2020

Run Java KafkaIO Performance Test

@iemejia
Copy link
Member Author

iemejia commented Feb 25, 2020

Fixes done PTAL @aromanenko-dev

@aromanenko-dev
Copy link
Contributor

Run Java PreCommit

@aromanenko-dev
Copy link
Contributor

Run Java KafkaIO Performance Test

Copy link
Contributor

@aromanenko-dev aromanenko-dev left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@aromanenko-dev aromanenko-dev merged commit 827ad19 into apache:master Feb 25, 2020
@iemejia
Copy link
Member Author

iemejia commented Feb 25, 2020

Thanks !

@iemejia iemejia deleted the BEAM-9364-kafkaio-deserializerproviders branch February 25, 2020 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants