Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

Fix Spark Dispatcher's image config #179

Merged
merged 1 commit into from
Sep 18, 2017

Conversation

skonto
Copy link
Contributor

@skonto skonto commented Sep 4, 2017

The property service.docker-image in config.json is not used. This is a nice property which helps people use a spark image other than the one provided already by Mesosphere.
Tested by deriving an image:

FROM mesosphere/spark:1.1.0-2.1.1-hadoop-2.6
ADD mesos-cluster-dispatcher.properties /opt/spark/dist/conf/mesos-cluster-dispatcher.properties
WORKDIR /opt/spark/dist

@skonto
Copy link
Contributor Author

skonto commented Sep 4, 2017

@susanxhuynh @ArtRand pls review.

@susanxhuynh
Copy link
Contributor

LGTM, cc @ArtRand

@ArtRand
Copy link
Contributor

ArtRand commented Sep 5, 2017

@skonto would it be possible to add a unit test to this? I'm already nervous enough about the poor test coverage in this repo.

@skonto
Copy link
Contributor Author

skonto commented Sep 5, 2017

@ArtRand sure I will have a look could you point me to where do you usually put such tests?
I don't see any universe tests only spark level tests. How should I test this?

@ArtRand
Copy link
Contributor

ArtRand commented Sep 6, 2017

@skonto the tests usually go in spark-build/tests/test_spark.py, and any files (like a new Dockerfile) go in spark-build/tests/resources. It may be a little tricky/annoying to build another derived imaged as park of the jenkins job (you'd have to change the web-of-scripts around spark-build/bin/jenkins-package-test.sh). If so maybe we can skip it. What do you think @susanxhuynh?

@susanxhuynh
Copy link
Contributor

Having test coverage is good in general. Building a derived image is tricky: you'd have to generate the Dockerfile on the fly if you wanted to derive from the latest image.

@skonto
Copy link
Contributor Author

skonto commented Sep 8, 2017

@susanxhuynh @ArtRand should I go through this for such a trivial modification? If I were to do this it makes sense to do it for all the settings there... I think it is too much, we can always improve coverage in another PR, thoughts?

@ArtRand
Copy link
Contributor

ArtRand commented Sep 18, 2017

LGTM. @susanxhuynh if you're ok with this give it a merge? (just running the tests once more because it's been a while)

@ArtRand
Copy link
Contributor

ArtRand commented Sep 18, 2017

retest this please

@susanxhuynh susanxhuynh merged commit a1d3d4c into d2iq-archive:master Sep 18, 2017
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.

3 participants