SPARK-14421 : Upgrade Kinesis Client Library (KCL) to 1.6.2, fixes support for de-aggregation. #12209

boneill42 · 2016-04-06T16:30:13Z

What changes were proposed in this pull request?

Upgrades KCL dependency, backported to 1.6.1.

How was this patch tested?

Used kinesis word count example against a stream containing aggregated data.

(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

See: SPARK-14421

srowen · 2016-04-06T17:09:41Z

pom.xml

    <log4j.version>1.2.17</log4j.version>
    <hadoop.version>2.2.0</hadoop.version>
-    <protobuf.version>2.5.0</protobuf.version>
+    <protobuf.version>2.6.1</protobuf.version>


This isn't necessarily OK since it changes protobuf for the whole project, and could conflict with versions used by, say, Hadoop. 2.4 and 2.5 were terribly incompatible, and I am paranoid about 2.5 vs 2.6.

The KCL update itself is probably fine. Is that helpful at all by itself? you'll have to update the deps files to reflect the updated dependencies for tests to pass.

Agreed... definitely needs testing.

Just upgrading the KCL didn't work. The new KCL relies on the following class from protobuf: com/google/protobuf/ProtocolStringList.class, which doesn't exist until 2.6.1.

srowen · 2016-04-06T17:10:13Z

If it turns out this will move forward, you need to make a JIRA and connect it. https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark

boneill42 · 2016-04-06T17:29:22Z

I had created a JIRA:
https://issues.apache.org/jira/browse/SPARK-14421

srowen · 2016-04-06T17:47:39Z

Yeah, this could be tough if the problem is really a protobuf difference. You need to update the title of this PR. See the link above.

boneill42 · 2016-04-06T17:58:33Z

Agreed. I just wanted to get it on the radar. People might experience sporadic problems consuming from kinesis streams until we can get it in there.

(title updated)

srowen · 2016-04-06T18:17:44Z

If you're actively working on this, that's fine, but if ultimately we can't merge this change because of protobuf then the PR should be closed. It stays around as does the JIRA

boneill42 · 2016-04-06T18:25:35Z

I'm not actively working it. We'll likely just fork and build a custom kinesis-asl jar.

Does it make sense to upgrade the dependencies for kinesis-asl? (and not touch the parent pom?)

srowen · 2016-04-06T18:34:03Z

I looked at the KCL dependencies, and 1.4.0 already depended on protobuf 1.6.1. What was the error you got -- clearly a version mismatch problem? just checking that it now becomes unavoidable but somehow worked before.

Yes, it might be viable to explicitly depend on protobuf 1.6.1 only in the KCL assembly. Add a version and un-mark it as provided and add a comment. That could be interesting to try. Although there's a risk of conflict in theory, it sounds like it at least fixes a real problem in practice now.

boneill42 · 2016-04-06T18:51:19Z

Here is the full stack:

Caused by: java.lang.NoClassDefFoundError: com/google/protobuf/ProtocolStringList
    at com.amazonaws.services.kinesis.clientlibrary.types.UserRecord.deaggregate(UserRecord.java:235)
    at com.amazonaws.services.kinesis.clientlibrary.lib.worker.ProcessTask.call(ProcessTask.java:146)
    at com.amazonaws.services.kinesis.clientlibrary.lib.worker.MetricsCollectingTaskDecorator.call(MetricsCollectingTaskDecorator.java:49)
    at com.amazonaws.services.kinesis.clientlibrary.lib.worker.MetricsCollectingTaskDecorator.call(MetricsCollectingTaskDecorator.java:24)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.ClassNotFoundException: com.google.protobuf.ProtocolStringList
    at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
    at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:331)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:357)

I'll see if I can perform surgery at the kinesis-asl level and leave the top pom alone.

…is-asl-assembly level.

boneill42 · 2016-04-06T19:17:29Z

Yep, that worked (and updated PR to reflect changes)... but also acknowledge that there is a chance for conflict since both 2.5.0 and 2.6.1 will be in play.

srowen · 2016-04-07T08:10:22Z

Summoning @vanzin to ask if this would indeed only affect the Kinesis artifacts? if that's all it affects and it fixes a problem, seems OK. Seems risky of course to modify protobuf any more widely.

vanzin · 2016-04-07T18:14:35Z

This would only affect the kinesis artifacts. But to actually use that at runtime, don't you need to relocate the classes in the assembly?

Otherwise, when you add the kinesis assembly to an app with "--packages", by default it will try to load protobuf from the parent class loader, which might still contain the old version.

boneill42 · 2016-04-07T18:39:26Z

That was my fear as well. Right now, we are adding the kinesis-asl-assembly jar via spark-defaults.conf.

spark.driver.extraClassPath /Users/brianoneill/tools/spark/spark-streaming-kinesis-asl-assembly_2.10-1.6.1-monetate.jar
spark.executor.extraClassPath /Users/brianoneill/tools/spark/spark-streaming-kinesis-asl-assembly_2.10-1.6.1-monetate.jar

That appears to work, but it could be luck. It could be that some of the 2.5.0 and 2.6.1 classes are compatible, or a 2.6.1-specific class is getting loaded first. I'm not familiar enough with how Spark constructs the classpaths.

Note also that we are running this from PySpark.

Here is a gist with the example that fails w/ KCL 1.4.0, but works with 1.6:
https://gist.github.com/boneill42/020dde814346c6b4ad0ba28406c3ea10

If you add the assembly to the classpath as above, you can run this gist with:

python dump.py boneill-2 my-stream https://kinesis.us-east-1.amazonaws.com us-east-1

vanzin · 2016-04-07T18:48:59Z

Adding the assembly to the classpath that way means the newer protobuf will override any older version on the Spark jars. So kinesis will work, but any code in Spark that is not compatible with the new protobuf will fail. It sounds a little risky.

Doing relocation in the kinesis assembly should be pretty straight forward. Here's an example from Spark's own build: https://github.com/apache/spark/blob/master/pom.xml#L2207

boneill42 · 2016-04-07T19:39:34Z

Ah, brilliant. I just tested out the relocation and it appeared to work. (PR updated)

vanzin · 2016-04-07T23:10:05Z

extras/kinesis-asl-assembly/pom.xml

+        <relocations>
+            <relocation>
+              <pattern>com.google.protobuf</pattern>
+              <shadedPattern>kcl.protobuf</shadedPattern>


to be consistent, the package name should be ${spark.shade.packageName}.kcl.protobuf(or kinesis.protobuf? kcl is not immediately obvious).

vanzin · 2016-04-07T23:10:22Z

Just a naming suggestion but otherwise this LGTM.

boneill42 · 2016-04-08T12:57:34Z

Good point. I updated to kinesis.protobuf. (I wanted to stay away from any potential collision with the parent, which is why I didn't re-used the spark.shade.packageName)

srowen · 2016-04-08T14:10:19Z

extras/kinesis-asl-assembly/pom.xml

          </includes>
        </artifactSet>
+        <relocations>
+            <relocation>


Nit: this section is indented 2 spaces too far, though it won't matter

srowen · 2016-04-08T14:10:31Z

Jenkins test this please

SparkQA · 2016-04-08T14:13:33Z

Test build #55346 has finished for PR 12209 at commit fe98fc1.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-04-08T14:17:03Z

Right, you'll also have to update the build/deps files to reflect this. dev/test-dependencies.sh can help.

boneill42 · 2016-04-08T14:57:34Z

@srowen ok, i'm able to run dev/test-dependencies.sh.
I had to manually download mvn and put it in the right place though.
The script didn't give me any errors.

In looking at the Jenkins output, it looks like it can't find mvn. (exactly what I ran into)

build/mvn: line 152: /home/jenkins/workspace/SparkPullRequestBuilder/build/apache-maven-3.3.3/bin/mvn: No such file or directory

Is that just a red herring?

I can proceed and just add the deps, but I'm not sure how to test it.

srowen · 2016-04-08T15:05:59Z

build/mvn will download Maven 3.3.9. You may need --force to override your local install. 3.3.3 is the wrong version.

boneill42 · 2016-04-08T15:12:28Z

Locally, i'm okay.

I'm seeing the 3.3.3 error in the Jenkins build:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/55346/console

srowen · 2016-04-08T15:27:26Z

A-ha, well, two problems here. The underlying cause is that it can't download Maven because the ASF site is in maintenance mode now. Try http://archive.apache.org/dist/maven/maven-3/3.3.3/binaries/apache-maven-3.3.3-bin.tar.gz

But I was surprised to see 3.3.3 since we are on 3.3.9 in master, and I see now you opened the PR vs the 1.6 branch. This should be opened vs master, and then it is back-ported as needed.

boneill42 · 2016-04-08T15:30:15Z

ah, got it. I will re-open a new against master. (and reference this one)

srowen · 2016-04-12T08:06:02Z

@boneill42 if you'll reopen vs master I'll merge it

boneill42 · 2016-04-15T13:21:13Z

@srowen it looks like master is already migrated to kinesis consumer library v 1.6.2:
https://github.com/apache/spark/blob/master/pom.xml#L150

Can that be backported safely to 1.6.x?

srowen · 2016-04-15T13:36:11Z

@boneill42 Aha right. That indicates that updating the library actually entailed some significant dependency changes. I don't know if that's OK to back-port to 1.6, but it's already in master. Presumably master needs your additional changes here to resolve your issue, and you could bump to 1.6.2 while you're at it.

srowen · 2016-04-22T13:48:36Z

@boneill42 do you want to take a crack at adding this change and bumping to 1.6.2 in master, so that it gets into 2.0?

boneill42 · 2016-04-22T17:34:34Z

yeah -- sorry, been busy with the day job.

I'll move this to master.

boneill42 · 2016-05-11T17:29:44Z

OK -- I just confirmed that I still see this issue on master.

    Caused by: java.lang.ClassNotFoundException: com.google.protobuf.ProtocolStringList

I'll submit a patch now.

boneill42 · 2016-05-11T17:55:43Z

New PR available here:
#13054

Upgraded KCL version to support de-aggregation of Kinesis records.

5395ad0

See: SPARK-14421

srowen reviewed Apr 6, 2016
View reviewed changes

boneill42 changed the title ~~Upgraded KCL version to support de-aggregation of Kinesis records.~~ SPARK-14421 Apr 6, 2016

Revering parent protobuf version, moving to explicit version at kines…

c823544

…is-asl-assembly level.

Brian O'Neill added 2 commits April 7, 2016 15:25

Adding relocation, as not to step on protobuf 2.5.0 from Spark pom

5705a70

Protobuf relocation

7a54d33

boneill42 changed the title ~~SPARK-14421~~ SPARK-14421 : Upgrade Kinesis Client Library (KCL) to 1.6.2, fixes support for de-aggregation. Apr 7, 2016

vanzin reviewed Apr 7, 2016
View reviewed changes

Updating package name to make it obvious what this package is for.

fe98fc1

srowen reviewed Apr 8, 2016
View reviewed changes

Fixing indentation

b393559

boneill42 closed this Apr 8, 2016

SPARK-14421 : Upgrade Kinesis Client Library (KCL) to 1.6.2, fixes support for de-aggregation. #12209

SPARK-14421 : Upgrade Kinesis Client Library (KCL) to 1.6.2, fixes support for de-aggregation. #12209

Uh oh!

Conversation

boneill42 commented Apr 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

boneill42 Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

srowen commented Apr 6, 2016

Uh oh!

boneill42 commented Apr 6, 2016

Uh oh!

srowen commented Apr 6, 2016

Uh oh!

boneill42 commented Apr 6, 2016

Uh oh!

srowen commented Apr 6, 2016

Uh oh!

boneill42 commented Apr 6, 2016

Uh oh!

srowen commented Apr 6, 2016

Uh oh!

boneill42 commented Apr 6, 2016

Uh oh!

boneill42 commented Apr 6, 2016

Uh oh!

srowen commented Apr 7, 2016

Uh oh!

vanzin commented Apr 7, 2016

Uh oh!

boneill42 commented Apr 7, 2016

Uh oh!

vanzin commented Apr 7, 2016

Uh oh!

boneill42 commented Apr 7, 2016

Uh oh!

vanzin Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

vanzin commented Apr 7, 2016

Uh oh!

boneill42 commented Apr 8, 2016

Uh oh!

srowen Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

srowen commented Apr 8, 2016

Uh oh!

SparkQA commented Apr 8, 2016

Uh oh!

srowen commented Apr 8, 2016

Uh oh!

boneill42 commented Apr 8, 2016

Uh oh!

srowen commented Apr 8, 2016

Uh oh!

boneill42 commented Apr 8, 2016

Uh oh!

srowen commented Apr 8, 2016

Uh oh!

boneill42 commented Apr 8, 2016

Uh oh!

srowen commented Apr 12, 2016

Uh oh!

boneill42 commented Apr 15, 2016

Uh oh!

srowen commented Apr 15, 2016

Uh oh!

srowen commented Apr 22, 2016

Uh oh!

boneill42 commented Apr 22, 2016

Uh oh!

boneill42 commented May 11, 2016

boneill42 commented Apr 6, 2016 •

edited

Loading