Kafka 0.10 support #282

tqh · 2016-08-17T11:43:18Z

Our changes to work with Kafka 0.10. It's not well tested, but seems to work ok.
Not tested at all is:

using Kafka client 0.10 for a Kafka 0.9 cluster
LogKafka

This is just to get the ball rolling...

…sting.

yahoocla · 2016-08-17T11:43:18Z

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

yahoocla · 2016-08-17T14:04:12Z

CLA is valid!

mckn · 2016-09-06T09:12:41Z

To start, good work with this project! Any plans on when this pull request will be accepted into master?

magaton · 2016-09-15T09:27:57Z

Hello, could you please tell when this will be merged. We are going with 0.10.0.1 in prod very soon and we desperately need a good monitoring tool. Thank you.

chrisfabri · 2016-09-16T04:27:54Z

dido

bai · 2016-09-16T12:39:24Z

Hey folks — any chance on getting this merged sometime soon? And thanks @tqh for pushing this, really helpful.

davemcphee · 2016-09-16T15:42:03Z

Just FYI, I've been running this pull in prod for 2 weeks now with great success. Until it's merged:

git clone https://github.com/yahoo/kafka-manager.git
git fetch origin pull/282/head:0.10.0
git checkout 0.10.0

many thanks to @tqh !

whithajess · 2016-09-18T23:22:03Z

app/kafka/manager/actor/cluster/KafkaStateActor.scala

@@ -295,7 +295,7 @@ case class KafkaManagedOffsetCache(clusterContext: ClusterContext
              lastUpdateTimeMillis = System.currentTimeMillis()
            } catch {
              case e: Exception =>
-                warn("Failed to process a message from offset topic!", e)
+                warn(s"Failed to process a message from offset topic on cluster ${clusterContext.config.name}!", e)


Generally wouldn't look for something in a Exception case as it may not exist so maybe the ${clusterContext.config.name} should be removed

It already proved very convenient to me. Why do you want to hide helpful information?

Do not want to hide it, but if you access a variable that doesn't exist in the Exception case then what will catch that exception?

Sorry, but this review isn't helpful.

@whithajess, in general, you have a point that if a variable is undefined in a catch, then there's no point in catching the first exception if we just generate another one anyway. However, in this case ${clusterContext.config.name} is already used on line 242, and everything is wrapped in a try anyway. This change wouldn't be executed if ${clusterContext.config.name} wasn't defined.

whithajess

Maybe some of these changes are just to do with the libraries, but it does look like this change would break a lot of backwards compatibilities. Was this tested with anything but 0.10?

whithajess · 2016-09-18T23:27:00Z

build.sbt

@@ -35,7 +35,7 @@ libraryDependencies ++= Seq(
  "org.slf4j" % "log4j-over-slf4j" % "1.7.12",
  "com.adrianhurt" %% "play-bootstrap3" % "0.4.5-P24",
  "org.clapper" %% "grizzled-slf4j" % "1.0.2",
-  "org.apache.kafka" %% "kafka" % "0.9.0.1" exclude("log4j","log4j") exclude("org.slf4j", "slf4j-log4j12") force(),


Surely this 0.9.0.1 part is still needed for older support.

Kafka client is backwards complient with 0.9. Havn't checked for older versions. Tested with a 0.9 cluster.

whithajess · 2016-09-18T23:28:39Z

test/kafka/test/SeededBroker.scala


-  def send(message: String, partition: String = null): Unit = send(message.getBytes("UTF8"), if (partition == null) null else partition.getBytes("UTF8"))


again this else partition.getBytes("UTF8") removal looks like it would break backwards compatibility

Kafka 0.10 has a different API.

rosarioleo · 2016-09-19T10:15:35Z

Lotlir

On Wed, Aug 17, 2016, 1:43 PM Yahoo CLA Bot notifications@github.com
wrote:

Thank you for submitting this pull request, however I do not see a valid
CLA on file for you. Before we can merge this request please visit
https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#282 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AK6ocsewlZ9YIFPTQR7B5EsjdoCDKYgmks5qgvPjgaJpZM4JmXIf
.

rosscdh · 2016-09-23T08:01:00Z

+1 merge

deepshikhagandhi · 2016-09-23T14:16:08Z

+1 please merge

vgrivel · 2016-09-27T12:33:40Z

I'm also waiting for the merge
+1
Thanks

JordyMoos · 2016-09-30T14:32:50Z

Is there any progress on this PR?

salyh · 2016-10-05T07:02:41Z

I can confirm that this PR works great with Kafka 0.10. We use since two weeks now without any problems.

salyh · 2016-10-05T07:03:07Z

+1 please merge

kevinleeTCA · 2016-10-06T05:47:24Z

+1 nice work

magaton · 2016-10-13T10:11:05Z

please...

padilo · 2016-10-18T08:38:19Z

I can confirm works with 0.10, but I had trouble on a 0.8.2 cluster using this version... It accepts the configuration but it doesn't see any broker in my cluster.

Anyone tested against other versions?

junkiebev · 2016-10-21T21:14:35Z

+1 please

kildievr · 2016-12-22T12:35:00Z

+1 please merge

antkudr · 2016-12-22T12:35:16Z

+1

mikhailmulyar · 2016-12-22T12:56:00Z

+1

lpicanco · 2016-12-23T17:47:34Z

Can someone merge?

zhan-yl · 2016-12-26T06:22:18Z

+1 please merge

hubian · 2016-12-28T03:50:58Z

please merge+1

Renkai · 2017-01-04T03:53:09Z

please merge +1

techwhizbang · 2017-01-04T22:07:28Z

if I'm reading correctly the reason it hasn't been merged is b/c of the Yahoo CLA agreement not being signed. did the author of this PR already sign it? @tqh?

tqh · 2017-01-04T22:11:54Z

@techwhizbang You are not reading it correctly. I signed ages ago. It's because Yahoo still uses older Kafka from what I understand. The 'all checks have passed' has a check for CLA.

shemeemsp7 · 2017-01-06T11:07:09Z

please merge

hgfischer · 2017-01-09T17:24:42Z

+💯

yanspirit · 2017-01-17T04:13:10Z

please merge+1

arisu1000 · 2017-01-17T08:18:27Z

please merge+1

sainat · 2017-01-19T22:06:32Z

+1
please merge

massimodeluisa · 2017-01-20T16:38:52Z

+1 please merge

natewarr · 2017-01-23T20:54:54Z

Has anyone tried this on 10.1.1?

patelh · 2017-01-23T21:34:40Z

Anyone tried this on 0.8.x, 0.9.x to check backwards compatibility?

padilo · 2017-01-24T14:36:21Z

I checked on Oct 18, 2016, and at least with 0.8.2 it doesn't work. I can have a look with current status but only with 0.8.2 cluster.

In a couple of hours I'll say something

sainat · 2017-01-24T19:04:24Z

@natewarr I tested this branch on a single node kafka 0.10.1.1 broker. Set kafka version to 0.10.1.0, no issues so far. I am going to deploy a multi broker 0.10.1.1 cluster pretty soon, will keep you posted on how it goes. It works with 0.9.0.1 (multi broker and single broker cluster).

padilo · 2017-01-26T09:34:38Z

Sorry for the delay, I had some fires and I didn't have time to check it until now.

@patelh I can confirm It works on a 0.8.2 cluster.

gumartinm · 2017-01-27T10:54:24Z

@natewarr It also works on 0.10.1.1.

rmangi · 2017-02-08T20:44:07Z

Would love to have this merged in 👍

ackramer · 2017-02-08T21:04:45Z

+1 yes please

khou · 2017-02-20T07:05:48Z

🙇

patelh · 2017-02-23T06:37:49Z

Are we okay to merge this?

fholmqvist added 4 commits August 17, 2016 13:28

Update to kafka 0.10.0.1 kafka client.

c03682d

Include which cluster the warning is about.

dd92b03

MinHeaderSize was removed as it was same as MinMessageOverhead

5f20535

Enable Kafka 0.10.0.0 and 0.10.0.1. Seems to work ok, needs proper te…

f4d006d

…sting.

Fix deprecations by using new Java API.

d6e3513

qfissler mentioned this pull request Aug 25, 2016

support for 0.10.0.0? #238

Closed

vixns mentioned this pull request Aug 28, 2016

strange memory usage #286

Closed

cxmokai mentioned this pull request Sep 8, 2016

can not see consumer with kafka 0.10 #287

Open

Sync with yahoo/kafka-manager

2835dd5

whithajess reviewed Sep 18, 2016

View reviewed changes

csghuser mentioned this pull request Oct 5, 2016

Add support for Kafka 0.10+ #268

Open

slitayem mentioned this pull request Dec 22, 2016

add support for kafka 0.10.0.1 #319

Open

FreeSlaver mentioned this pull request Feb 9, 2017

Cannot see the consumer group #285

Open

patelh merged commit 555cdae into yahoo:master Feb 23, 2017

BDeus mentioned this pull request Apr 7, 2017

Is this project active? #350

Open

patelh referenced this pull request May 7, 2017

Update readme

dd80fd8


		def send(message: String, partition: String = null): Unit = send(message.getBytes("UTF8"), if (partition == null) null else partition.getBytes("UTF8"))

Kafka 0.10 support #282

Kafka 0.10 support #282

Conversation

tqh commented Aug 17, 2016

yahoocla commented Aug 17, 2016

yahoocla commented Aug 17, 2016

mckn commented Sep 6, 2016

magaton commented Sep 15, 2016

chrisfabri commented Sep 16, 2016

bai commented Sep 16, 2016

davemcphee commented Sep 16, 2016 • edited Loading

whithajess Sep 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whithajess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosarioleo commented Sep 19, 2016

rosscdh commented Sep 23, 2016

deepshikhagandhi commented Sep 23, 2016

vgrivel commented Sep 27, 2016

JordyMoos commented Sep 30, 2016 • edited Loading

salyh commented Oct 5, 2016

salyh commented Oct 5, 2016

kevinleeTCA commented Oct 6, 2016

magaton commented Oct 13, 2016

padilo commented Oct 18, 2016

junkiebev commented Oct 21, 2016

kildievr commented Dec 22, 2016

antkudr commented Dec 22, 2016

mikhailmulyar commented Dec 22, 2016

lpicanco commented Dec 23, 2016

zhan-yl commented Dec 26, 2016

hubian commented Dec 28, 2016

Renkai commented Jan 4, 2017

techwhizbang commented Jan 4, 2017

tqh commented Jan 4, 2017

shemeemsp7 commented Jan 6, 2017 • edited Loading

hgfischer commented Jan 9, 2017

yanspirit commented Jan 17, 2017

arisu1000 commented Jan 17, 2017

sainat commented Jan 19, 2017

massimodeluisa commented Jan 20, 2017

natewarr commented Jan 23, 2017

patelh commented Jan 23, 2017

padilo commented Jan 24, 2017 • edited Loading

sainat commented Jan 24, 2017 • edited Loading

padilo commented Jan 26, 2017

gumartinm commented Jan 27, 2017 • edited Loading

rmangi commented Feb 8, 2017

ackramer commented Feb 8, 2017

khou commented Feb 20, 2017

patelh commented Feb 23, 2017

davemcphee commented Sep 16, 2016 •

edited

Loading

whithajess Sep 18, 2016 •

edited

Loading

JordyMoos commented Sep 30, 2016 •

edited

Loading

shemeemsp7 commented Jan 6, 2017 •

edited

Loading

padilo commented Jan 24, 2017 •

edited

Loading

sainat commented Jan 24, 2017 •

edited

Loading

gumartinm commented Jan 27, 2017 •

edited

Loading