SPARK-11565 Replace deprecated DigestUtils.shaHex call

[gliptak]

No description provided.

[SparkQA]

Test build #45273 has finished for PR 9532 at commit 5e2370c.

• This patch fails MiMa tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):\n * abstract class Writer extends BaseReadWrite\n * trait Writable\n * abstract class Reader[T] extends BaseReadWrite\n * trait Readable[T]\n * case class GetInternalRowField(child: Expression, ordinal: Int, dataType: DataType)\n * case class Expand(\n

[gliptak]

The error message displayed seems to have no relation to the pull request change ...

The compile does fail with:

[error] /home/jenkins/workspace/SparkPullRequestBuilder/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:127: value sha1Hex is not a member of object org.apache.commons.codec.digest.DigestUtils
[error]     UTF8String.fromString(DigestUtils.sha1Hex(input.asInstanceOf[Array[Byte]]))
[error]                                       ^
[error] one error found

although <commons-codec.version>1.10</commons-codec.version>

[srowen]

Eh, the line you changed doesn't compile. It seems quite directly related! But yes the question is why indeed. Does it compile for you with Maven? then we may have a Maven/SBT resolution difference again and you are getting older Codec from Hadoop probably in the SBT build. You may have to adjust dependencies to force some component to not bring in its version of Codec. I think this is why I wasn't able to change this before.

[gliptak]

@srowen From SparkQA: "This patch adds the following public classes (experimental):" I do not see how my change adds a public class ...
As for the compile error, it does build with `mvn`` locally.

[srowen]

@gliptak you can ignore that, it's indeed a false positive. The problem is the compilation error. Yes, the problem is the SBT build, not Maven, as expected. You'd have to investigate why the SBT dep resolution differs and adjust exclusions. It's annoying but this is the price of trying to make SBT work as well.

[gliptak]

@srowen I'm running build/sbt dependency-tree and it is literally running for several hours now. Is there an easier way to work this (or is there something to correct in my setup)? Thanks

[srowen]

That doesn't sound right. Is it stuck somewhere? alternatively you might be able to deduce the problem with mvn -Dverbose dependency:tree as it will show you what older versions are being replaced with newer versions, and you might see where a version < 1.6 is coming in and needs to be excluded explicitly.

[SparkQA]

Test build #45303 has finished for PR 9532 at commit 7b7e075.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds no public classes.

[gliptak]

@srowen Which maven pom.xml this corresponds to (streaming?)?

[error] (streaming-kinesis-asl-assembly/*:update) java.lang.IllegalStateException: impossible to get artifacts when data has not been loaded. IvyNode = commons-codec#commons-codec;1.4
[error] (streaming-kafka-assembly/*:update) java.lang.IllegalStateException: impossible to get artifacts when data has not been loaded. IvyNode = commons-codec#commons-codec;1.4

[srowen]

These are in extras, and external, respectively. That's a strange error, but you may be onto something. What does mvn -Dverbose dependency:tree show over here? old versions of codec being replaced? that's what we may have to patch.

[SparkQA]

Test build #45323 has finished for PR 9532 at commit 7528029.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[gliptak]

Number of errors related to signatures:

https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/45323/testReport/org.apache.spark/JavaAPISuite/reduce/

Error Message

class "javax.servlet.FilterRegistration"'s signer information does not match signer information of other classes in the same package

Stacktrace

sbt.ForkMain$ForkError: class "javax.servlet.FilterRegistration"'s signer information does not match signer information of other classes in the same package
    at java.lang.ClassLoader.checkCerts(ClassLoader.java:952)
    at java.lang.ClassLoader.preDefineClass(ClassLoader.java:666)
    at java.lang.ClassLoader.defineClass(ClassLoader.java:794)
    at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
    at java.net.URLClassLoader.defineClass(URLClassLoader.java:449)

Something got reordered with the pom.xml modifications?

[srowen]

You don't need to re-declare the dependency and indeed don't want to; you're duplicating version info

[gliptak]

The reason to bring it in directly is those exclusions above. I will validate thus once more.

[gliptak]

Without this, the other exclusions drop commons-codec.

[srowen]

That means something is bringing in javax.servlet now, which must be excluded because it resists repackaging. It's a rabbit hole; it'd be great to bottom out as that might be it. In the worst case we might have to give up on this as an example of how SBT doesn't resolve things the same way.

[gliptak]

@srowen So you are seeing this as a side effect if the Maven dependency changes ... I will work this some more (locally).

[gliptak]

#3864

[gliptak]

@srowen Can you offer some pointers on how to move this forward?

[srowen]

Hm, I still think you don't want to duplicate the version info. I'm surprised if you have to exclude and then add dependencies to get SBT working. What about only depending on the component directly? No exclusion.

[gliptak]

@srowen I'm working this using mvn I will remove one of the excludes to bring the dependency back (without direct reference)

[yhuai]

@srowen Looks like hadoop is not using the same version of common-code? Should we bump the version of commons-codec just for using sha1Hex()?

[srowen]

@yhuai in the past, the problem was that it then fails when an older version is provided by old Hadoop 1.x at runtime. Hadoop 2.2+ has a recent enough version (1.7). However I encountered a problem with how MiMa executes when trying to change this that I didn't fully understand. You can try managing it up to version 1.10 and see the problem.

[yhuai]

(you meant "You can try managing it up to version 1.7"?)

[srowen]

I was figuring go all the way, but sure, 1.7 is a little more conservative. The Commons libraries don't tend to remove old deprecated methods, which would be the problem in updating past 1.7.

[gliptak]

Would you like me to update this patch with 1.7?

[srowen]

Right now the issues seem to be: you're duplicating the version info in child POMs, and adding exclusions that probably aren't needed. Transitive dependencies are managed up by Maven. Version 1.10 seems OK to me.

[srowen]

@gliptak are you able to follow up on this?

[gliptak]

@srowen This built locally.

[SparkQA]

Test build #49896 has finished for PR 9532 at commit a22b0b8.

• This patch fails PySpark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #2439 has finished for PR 9532 at commit a22b0b8.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

These tests are failing everywhere, it's not related

[JoshRosen]

Jenkins, retest this please

[SparkQA]

Test build #49957 has finished for PR 9532 at commit a22b0b8.

• This patch fails PySpark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

Jenkins, retest this please

[gliptak]

Github is hiccuping ...

[srowen]

Jenkins, retest this please

[SparkQA]

Test build #50273 has finished for PR 9532 at commit a22b0b8.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

I'm sort of surprised that it was not necessary to update the file listing all of the project dependencies?

The change is looking fairly good, except that the version needs to be managed in dependencyManagement in the parent POM. If the sql module does not already depend on commons-codec it needs to, but core does not need a change.

[gliptak]

@srowen commons-codec is already in the section

[srowen]

I see (not in front of the code now) - should just be an edit to that section then. Not a new declaration in core. The change is in SQL anyway

[gliptak]

@srowen if you are proposed changing dependencies, would a follow-up JIRA work for that?

[srowen]

I think it is a necessary part of removing the deprecated call so that it works reliably?

[srowen]

Back online now. Actually, commons-codec is already declared at version 1.10 in the parent pom.xml. At this stage, we only need to change the deprecated call right?

[gliptak]

The deprecated call was updated with this commit.

[srowen]

@gliptak yes obviously. But as per several comments in this discussion, I still don't see why you added the pom stanzas that you did. I think these should be removed.

[gliptak]

@srowen Yes I removed the pom.xml reference, and was successful building locally. Maven dependencies might have changed since I initially issued the patch ...

[SparkQA]

Test build #50877 has finished for PR 9532 at commit 56f32b4.

• This patch fails MiMa tests.
• This patch merges cleanly.
• This patch adds no public classes.

[gliptak]

OK, back to my initial confusion on this. So the pom.xml change is needed to make this work ...

[srowen]

Yeah, this is what I saw too. It's not a problem with the build of the code itself, since as you say it passes locally. It's a problem that occurs when MiMa builds previous code to evaluate API differences. I didn't quite understand the fix. Your change is a clue; it may relate to Maven's closest-first dependency rules.

In that case, I suspect the right place to add the explicit dependency is in sql, not core. That's where it used so technically this must be declared.

In any event you don't need to repeat the version, and in fact should not.

[SparkQA]

Test build #50901 has finished for PR 9532 at commit 80dcf1f.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[gliptak]

@srowen Two tests failed with timeout. Unrelated?

[srowen]

Yes this test is bad at the moment. Good news is it got past MiMa tests, I think. The change looks right to me.

[srowen]

Jenkins, retest this please

[SparkQA]

Test build #50970 has finished for PR 9532 at commit 80dcf1f.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

Merged to master