Skip to content

Fix lz4 library incompatibility in kafka-indexing-service extension#4115

Merged
gianm merged 3 commits intoapache:masterfrom
satishbhor:master
Apr 25, 2017
Merged

Fix lz4 library incompatibility in kafka-indexing-service extension#4115
gianm merged 3 commits intoapache:masterfrom
satishbhor:master

Conversation

@satishbhor
Copy link
Copy Markdown
Contributor

@satishbhor satishbhor commented Mar 25, 2017

Added support for Kafka 0.10.0.0 lz4 so that #3266 issue can be resolved.

@satishbhor satishbhor changed the title Fix lz4 library incompatibility in kafka-indexing-service extension #… Fix lz4 library incompatibility in kafka-indexing-service extension #3266 Mar 25, 2017
@satishbhor satishbhor changed the title Fix lz4 library incompatibility in kafka-indexing-service extension #3266 Fix lz4 library incompatibility in kafka-indexing-service extension Mar 25, 2017
Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

This will break compatibility with 0.9 brokers, but I suppose I'm okay with that. Kafka makes it tough to support old versions since they keep breaking wire compatibility, so I guess there's not much we can do.

this extension is still experimental so it's ok to make the change in a point release.

@satishbhor could you please update the Kafka indexing docs to reflect that users' brokers must be 0.10.x or higher?

LGTM after that and the version bump to 0.10.2.0 (assuming that version works too)

<groupId>org.apache.kafka</groupId>
<artifactId>kafka_2.11</artifactId>
<version>0.9.0.0</version>
<version>0.10.0.0</version>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

0.10.2.0 is the latest, please use that if it works.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@gianm Thanks! I will update Kafka indexing service document and will also check with Kafka 0.10.2.0

@gianm gianm added this to the 0.10.1 milestone Mar 25, 2017
@gianm gianm requested a review from pjain1 March 25, 2017 23:03
@gianm
Copy link
Copy Markdown
Contributor

gianm commented Mar 25, 2017

@satishbhor Btw, if you haven't already filled out a CLA, could you please do so at: http://druid.io/community/cla.html

@satishbhor
Copy link
Copy Markdown
Contributor Author

satishbhor commented Mar 26, 2017

@gianm with minor changes in unit test cases it is working fine with Kafka 0.10.2.0 and also updated Kafka indexing service 0.10.x note in docs.

final long nextOffset = outOfRangePartition.getValue();
// seek to the beginning to get the least available offset
consumer.seekToBeginning(topicPartition);
consumer.seekToBeginning(Lists.newArrayList(topicPartition));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could use Collections.singletonList()?

Copy link
Copy Markdown
Contributor Author

@satishbhor satishbhor Apr 7, 2017

Choose a reason for hiding this comment

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

This file contains Lists.newArrayList() other places as well, so shall I replace those as well ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The other place uses Iterable as argument, it's a different case

Copy link
Copy Markdown
Contributor Author

@satishbhor satishbhor Apr 7, 2017

Choose a reason for hiding this comment

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

I mean to say statements like,

consumer.assign(Lists.newArrayList(topicPartition));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also in #4135 you mentioned prefer Collections.emptyList() over Arrays.asList() then shall i also replace Lists.newArrayList() with Collections.emptyList() ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It wasn't me, but yes, please

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@leventov my apologize Collections.emptyList() can't be used as lists are being manipulated in code.


if (useEarliestOffset) {
consumer.seekToBeginning(topicPartition);
consumer.seekToBeginning(Lists.newArrayList(topicPartition));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same

…x lz4 library incompatibility in kafka-indexing-service extension apache#4115
Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

👍 thanks for the contribution!

@gianm gianm merged commit d51097c into apache:master Apr 25, 2017
@satishbhor
Copy link
Copy Markdown
Contributor Author

Thanks @gianm for accepting changes.

gianm pushed a commit that referenced this pull request May 12, 2017
…gress (fixes #4214) (#4240)

* Fix lz4 library incompatibility in kafka-indexing-service extension #3266

* Bumped Kafka version to 0.10.2.0 for : Fix lz4 library incompatibility in kafka-indexing-service extension #3266

* Replaced Lists.newArrayList() with Collections.singletonList() For Fix lz4 library incompatibility in kafka-indexing-service extension #4115

* Fixed: Average Server Percent Used: NaN% Error when server startup is in progress #4214
seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Jan 10, 2020
…exing-service extension)

* Fix lz4 library incompatibility in kafka-indexing-service extension apache#3266

* Bumped Kafka version to 0.10.2.0 for : Fix lz4 library incompatibility in kafka-indexing-service extension apache#3266

* Replaced Lists.newArrayList() with Collections.singletonList() For Fix lz4 library incompatibility in kafka-indexing-service extension apache#4115
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.

3 participants