[SPARK-12961][CORE][FOLLOW-UP] Remove wrapper code for SnappyOutputStream

[maropu]

What changes were proposed in this pull request?

This pr removed the wrapper code (commit: f2cc6b5) to avoid the bug in SnappyOutputStream.close() because the corresponding bug has been fixed in snappy-java-1.1.2.6 that Spark currently uses. This fix has been merged in snappy-java-1.1.2 and see xerial/snappy-java#107 (comment).

How was this patch tested?

Checked UnsafeShuffleWriterSuite passed.

[maropu]

Probably, we forgot to remove this when upgrading snappy-java to 1.1.2.6. cc: @srowen @JoshRosen

[maropu]

Also, we could safely remove the code (commit: 5936bf9) to avoid memory leak in snappy-java? cc: @viirya This fix has been merged in snappy-java-1.1.2.1 (See: xerial/snappy-java#131 (comment))

[SparkQA]

Test build #80673 has finished for PR 18949 at commit c3f2470.

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

[kiszk]

LGTM

[viirya]

@maropu There is another reason we leave the workaround in place: #11524 (comment)

[viirya]

LGTM

[maropu]

@viirya aha, ok. thanks. (btw, since the comment is still important, we better keep it in code comment, maybe).

[maropu]

ping

[srowen]

Previously, the concern was that this workaround was still necessary because the runtime environment might have a different snappy version from the one Spark uses. I thought that was because Hadoop provides it, but, I am not sure that's true? I can't see a dependency on it in Hadoop, and mvn dependency:tree shows this as a compile-scope dependency at 1.1.2.6 for all of Spark. We don't explicitly pick it up from the env.

I think this is likely OK, but wonder if I'm missing something @viirya as you commented on it?

[viirya]

If I understand the previous comment #11524 (comment) correctly, seems it is happened before snappy-java get downgraded by user code when the user classpath takes precedence.

I am not sure where the snappy-java comes from, I don't know if it is Hadoop, maybe other library in user classpath? If we don't shade this dep, maybe we still keep it for safety?

[srowen]

Hm, if the risk is only that the user classpath might have an older version, then at some point that's an acceptable risk. There are many problems like that, and this version has been out for over a year.

Still, if it's not causing us problems except for a bit of extra code, we could also just err on the side of caution and push this out another 6-12 months or something.

[viirya]

Ok. It sounds reasonable to me.

[maropu]

Sounds good to me ,too.