Update Kafka connector to 2.3.1 by dcfocus · Pull Request #13709 · prestodb/presto

dcfocus · 2019-11-15T23:18:02Z

This PR improves Kafka connector implementation and performance.

Replaced SimpleConsumer with KafkaConsumer for Kafka connector.
Push collecting offsets for each partition from coordinator to workers to resolve the bottleneck.

dcfocus · 2019-11-15T23:20:20Z

@rongrong @zhenxiao
Refactored previous PR, this one is the main one with upgrading Kafka client. Other features will come later, including nested ROW type support and zookeeper discovery, etc.

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaThreadIdentifier.java

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaConsumerManager.java

zhenxiao

thank you, @dcfocus
I add some comments

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaConnectorConfig.java

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaConsumerManager.java

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaTableLayoutHandle.java

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaSplit.java

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaSplitManager.java

zhenxiao · 2019-11-27T23:52:32Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaTableLayoutHandle.java

s/offset_start_ts/startOffsetTimestamp/g
generally, we do not put underscore in toString

dcfocus · 2019-11-29T01:47:45Z

Hi @zhenxiao Thanks for all the comments. Very helpful!!
I fixed most of the comments and left explanation for the rest.
Can you take a look once you get time?
(Current build failed due to time exception of 1 hour 20 min. Will pass if re-run. But seems Travis doesn't allow re-run. )

OneCricketeer · 2019-11-29T04:39:12Z

pom.xml

Any reason this isn't higher?

Was to keep consistent with kafka_2.12

I'm not sure there's a correlation between the Scala version and Kafka version.

If Presto needs Scala 2.12, that's fine, but I mean, why not target Kafka 2.3.0, the latest version, for example?

Thanks Jordan for explanation. Yes, you are right. There shouldn't be correlation. I will update to latest compatible version.

same question, shall we use kafka 2.3.1, the latest version?

Right, but I keep running into this issue after I tried with 2.0.0+
Caused by: java.lang.ClassNotFoundException: org.apache.kafka.common.record.RecordFormat

OneCricketeer · 2019-11-29T04:42:00Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaConsumerManager.java

Should these be ByteBuffer if that's your deserializer type?

Good catch. Thanks for pointing out.

OneCricketeer · 2019-11-29T04:44:52Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaRecordSet.java

Should you add <ByteBuffer, ByteBuffer>?

OneCricketeer · 2019-11-29T04:49:58Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaSplitManager.java

Are types needed here?

dcfocus · 2019-12-04T22:44:50Z

This PR improves Kafka connector implementation and performance.

Replaced SimpleConsumer with KafkaConsumer for Kafka connector.

Push collecting offsets for each partition from coordinator to workers to resolve the bottleneck.

Removed change 2 after discussion with @zhenxiao . For most companies, kafka topic is not that big, we may not have significant performance gain from pushdown of getOffsetByTimeStamp.

zhenxiao

thank you, @dcfocus
A few more comments

zhenxiao · 2019-12-05T07:17:07Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaConsumerManager.java

s/props/properties/g

s/props/properties/g ?

zhenxiao · 2019-12-05T07:17:28Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaConsumerManager.java

static import these ConsumerConfig constants

zhenxiao · 2019-12-05T07:18:45Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaConsumerManager.java

why set classloader to null?

Fixed. Was there to get ride of some exception during experiments.

zhenxiao · 2019-12-05T07:23:29Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaRecordSet.java

use Thread.currentThread().getName(), instead of creating threadId

zhenxiao · 2019-12-05T07:27:13Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaRecordSet.java

I still feel 1s is too long, how about 500ms?

zhenxiao · 2019-12-05T07:29:52Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaSplitManager.java

use thread getName(), instead of getId()

zhenxiao · 2019-12-05T07:32:46Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaSplitManager.java

would it better return a pair of long? instead of having a long array?
Or, we could inline this functions

Good point. Inlined, fixed.

zhenxiao · 2019-12-05T07:34:12Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaSplitManager.java

move this line above the for loop

zhenxiao · 2019-12-05T07:37:19Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaStaticServerset.java

do we need KafkaCluster interface? It only has one implementation
we might just need one class with selectRandomServer()

zhenxiao · 2019-12-05T07:38:43Z

pom.xml

same question, shall we use kafka 2.3.1, the latest version?

zhenxiao

almost there, thank you @dcfocus
please update documentation:
./presto-docs/src/main/sphinx/connector/kafka.rst
./presto-docs/src/main/sphinx/connector/kafka-tutorial.rst

And your PR title should be: "Update Kafka connector to 2.3.1"

zhenxiao · 2019-12-06T19:26:07Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaConsumerManager.java

s/props/properties/g ?

zhenxiao · 2019-12-06T19:26:46Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaSplit.java

no need to move leader here, is it?

zhenxiao · 2019-12-06T19:35:46Z

Hi @Cricket007 upon tests passed, am mostly good with this PR
could you please do one more pass before I merge?

OneCricketeer

Mostly satisfied ✅

Minor comments

OneCricketeer · 2019-12-06T21:00:42Z

pom.xml

Could we extract this version as a property?

OneCricketeer · 2019-12-06T21:04:49Z

presto-kafka/src/test/java/com/facebook/presto/kafka/util/EmbeddedKafka.java

Default partitioner is default. Need to specify?

Fixed. Thanks for catching this one.

dcfocus · 2019-12-06T21:49:27Z

Hi @Cricket007 upon tests passed, am mostly good with this PR
could you please do one more pass before I merge?

Thanks to both. Very helpful comments.
One more change added: the notion segment is deprecated. I removed the related code in this commit.

zhenxiao

good to me
a few minor issues

zhenxiao · 2019-12-06T21:53:14Z

pom.xml

kafka.version

zhenxiao · 2019-12-06T21:53:32Z

pom.xml

just use kafka.version, no need to create a separate clients version

zhenxiao · 2019-12-06T21:54:41Z

presto-docs/src/main/sphinx/connector/kafka.rst

Apache kafka 2.3.1+ is supported

zhenxiao · 2019-12-06T21:55:54Z

presto-docs/src/main/sphinx/connector/kafka.rst

1024Kb * 1024 = 1MB?

zhenxiao

looks good to me
upon test passed, I will merge

zhenxiao

one more comment

zhenxiao · 2019-12-06T22:24:55Z

pom.xml

could we add this dependency to presto-kafka/pom.xml?

zhenxiao · 2020-01-13T10:41:50Z

@dcfocus could you please fix the test failures?

dcfocus · 2020-01-18T06:44:19Z

@dcfocus could you please fix the test failures?

Hi @zhenxiao The maximum run time for each check is limited to 1 hour 20 min. Sometimes it will finish within that limit. It doesn't allow me to re-run. Can you re-run it on your side?

zhenxiao · 2020-01-22T08:00:47Z

presto-server/pom.xml

why are we making these changes? this is for presto-server, better not to pollute the lib

OneCricketeer · 2020-01-24T01:03:04Z

presto-kafka/src/test/java/com/facebook/presto/kafka/util/TestUtils.java

I see nothing wrong with try with resources

beinan reviewed Nov 21, 2019

View reviewed changes

zhenxiao requested changes Nov 27, 2019

View reviewed changes

dcfocus requested a review from zhenxiao November 29, 2019 01:45

OneCricketeer reviewed Dec 4, 2019

View reviewed changes

zhenxiao requested changes Dec 5, 2019

View reviewed changes

zhenxiao requested changes Dec 6, 2019

View reviewed changes

dcfocus changed the title ~~Update Kafka Connector~~ Update Kafka connector to 2.3.1 Dec 6, 2019

OneCricketeer reviewed Dec 6, 2019

View reviewed changes

zhenxiao requested changes Dec 6, 2019

View reviewed changes

zhenxiao approved these changes Dec 6, 2019

View reviewed changes

zhenxiao reviewed Dec 6, 2019

View reviewed changes

pom.xml Outdated

Copy link

Collaborator

zhenxiao Dec 6, 2019

Choose a reason for hiding this comment

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

could we add this dependency to presto-kafka/pom.xml?

zhenxiao reviewed Jan 22, 2020

View reviewed changes

presto-server/pom.xml Outdated

Copy link

Collaborator

zhenxiao Jan 22, 2020

Choose a reason for hiding this comment

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

why are we making these changes? this is for presto-server, better not to pollute the lib

dcfocus force-pushed the oss-multi-commits/upgrade-to-kafkaconsumer branch from 523e809 to 8f77144 Compare January 22, 2020 22:00

dcfocus changed the base branch from release-0.228 to master January 22, 2020 22:08

dcfocus force-pushed the oss-multi-commits/upgrade-to-kafkaconsumer branch from 8f77144 to 687dc04 Compare January 22, 2020 22:28

OneCricketeer reviewed Jan 24, 2020

View reviewed changes

dcfocus force-pushed the oss-multi-commits/upgrade-to-kafkaconsumer branch 7 times, most recently from b6198ce to 1ac3805 Compare January 29, 2020 01:21

wenleix mentioned this pull request Jan 29, 2020

Upgrade Kafka Connector to 2.3.1 #14026

Closed

Upgrade Kafka Connector to 2.3.1

1dd8354

dcfocus force-pushed the oss-multi-commits/upgrade-to-kafkaconsumer branch from 1ac3805 to 1dd8354 Compare January 29, 2020 09:26

dcfocus mentioned this pull request Jan 29, 2020

Upgrade Kafka Connector to 2.3.1 #14030

Closed

zhenxiao merged commit 8eb0192 into prestodb:master Jan 30, 2020

caithagoras mentioned this pull request Feb 20, 2020

Add release notes for 0.232 #14130

Merged

8 tasks

Conversation

dcfocus commented Nov 15, 2019

Uh oh!

dcfocus commented Nov 15, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhenxiao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcfocus commented Nov 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcfocus commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhenxiao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcfocus commented Dec 4, 2019 •

edited

Loading