Skip to content
This repository was archived by the owner on Jan 9, 2020. It is now read-only.

Add libc6-compat to base spark docker image to fix issue with parquet… - #550

Open
echarles wants to merge 2 commits into
apache-spark-on-k8s:branch-2.2-kubernetesfrom
datalayer-externals:libc6-compat
Open

Add libc6-compat to base spark docker image to fix issue with parquet…#550
echarles wants to merge 2 commits into
apache-spark-on-k8s:branch-2.2-kubernetesfrom
datalayer-externals:libc6-compat

Conversation

@echarles

@echarles echarles commented Nov 12, 2017

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Add libc6-compat to base spark docker image to fix issue with parquet ld-linux-x86-64.so.2 not found

How was this patch tested?

With current docker images, basic dataframe manipulation works fine but saving to parquet format returns an exception du to missing linux library:

e-libsnappyjava.so: Error loading shared library ld-linux-x86-64.so.2:

This PR add to the spark-base image the needed Alpine package (libc6-compat) - Not sure if we want to make the base image more fat or if it would be better to target the 3 driver images.

To test, just rebuild and run List(1, 2, 3).toDS.write.parquet("/tmp/ds1")

@foxish

foxish commented Nov 14, 2017

Copy link
Copy Markdown
Member

@ssuchter @kimoonkim @ash211 Can you verify this?

@foxish

foxish commented Dec 6, 2017

Copy link
Copy Markdown
Member

cc/ @liyinan926 @mccheah
We need to verify this for the upstream docker images.

@liyinan926

Copy link
Copy Markdown
Member

rerun integration tests please

@echarles

Copy link
Copy Markdown
Member Author

I have run the integration tests with this change and I have the same random failures as described in #571.

IMHO this change does not impact the integration tests. The question is more Do we want to make the minimal bundles more fat than it is actually to support more usage patterns?

@ifilonenko

Copy link
Copy Markdown
Member

Thank you for your work on this!

@echarles echarles reopened this Dec 11, 2017
@foxish

foxish commented Dec 13, 2017

Copy link
Copy Markdown
Member

rerun integration tests please

@echarles

Copy link
Copy Markdown
Member Author

On the +/- 20 integration test runs I have run on my env, I had I think 2 runs with 100% green. The fully successful ones were not with this branch. Let's wait on a fix (wherever it is on my env or on the integration tests process) to merge.

@felixcheung

Copy link
Copy Markdown

this apache#19717 (comment) is saying libc6-compat doesn't fix the problem, do we have an alternative solution, and we need a fix up stream as well (soon)?

@foxish

foxish commented Jan 2, 2018

Copy link
Copy Markdown
Member

@felixcheung, I think we should switch the base image - since alpine is known to have such issues. ubuntu/centos might be good choices - but we should get that running in our branch first and tested before pushing upstream.

@echarles

echarles commented Jan 2, 2018

Copy link
Copy Markdown
Member Author

alpine or other lightweight base images are great as they start fast. I am fine switching to any other base, but we should ensure this does not introduce side-effects that we will only see in the future. @foxish To which extensive test are thinking to? If we can find some kind of acceptance tests, we could add them to the integration tests suites.

@foxish

foxish commented Jan 2, 2018

Copy link
Copy Markdown
Member

I was thinking more of having people test it out in prod - but yeah, we could write some integration testing as well. An set of integration tests marked as [conformance] to preserve existing behavior makes sense. We want those to work regardless of the base image chosen.

@echarles

echarles commented Jan 2, 2018

Copy link
Copy Markdown
Member Author

We can start with manual test (although it shows already its limits as it can work in some env, and not in others). Running those conformance tests (so not located in the integration repo/module) against various base images would be the target (although a lot of work).

@foxish

foxish commented Jan 2, 2018

Copy link
Copy Markdown
Member

sgtm. I think we should merge this change anyway because it's in upstream. @kimoonkim @ssuchter, can you PTAL at the integration tests? They seem wedged for some reason.

@felixcheung

felixcheung commented Jan 2, 2018 via email

Copy link
Copy Markdown

@foxish foxish closed this Jan 2, 2018
@foxish foxish reopened this Jan 2, 2018
@ssuchter

ssuchter commented Feb 5, 2018

Copy link
Copy Markdown
Member

retest this please

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants