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

Conversation

@hex108
Copy link

@hex108 hex108 commented Dec 26, 2017

What changes were proposed in this pull request?

Fix #584

How was this patch tested?

Manual tests as in #584.

@liyinan926
Copy link
Member

We already have org.apache.spark.deploy.k8s. HadoopConfBootstrapImpl that has the logic for mounting the HADOOP_FILE_VOLUME into a container.

@hex108
Copy link
Author

hex108 commented Dec 28, 2017

hi @liyinan926 , if I understand correctly, HadoopConfBootstrapImpl is used to mount Hadoop conf to driver/executor pod's main container, and now we only mount Hadoop conf to driver/executor container. However we also need mount it to init container to fetch HDFS dependencies.

@liyinan926
Copy link
Member

@hex108 Yes, it is for mounting the Hadoop conf ConfigMap into the driver and executor containers. My point is the logic is duplicated, so should be extracted out and shared. For example, we have MountSecretBootstrap that is shared for mounting secrets into the main and init containers in both the driver and executor pods. I would suggest renaming MountHadoopConfStep to something that has Bootstrap in the name and using it in places where mounting the ConfigMap is needed.

@hex108
Copy link
Author

hex108 commented Dec 28, 2017

@liyinan926 Thanks, I get it now. I'll refactor it.

@ifilonenko
Copy link
Member

ifilonenko commented Dec 28, 2017

If it is alright, I am currently working on integration tests for hdfs (secure and non), after a few LGTM and after passing the tests that I write, we can merge this PR, after resolving comments. Want to mitigate changes to HDFS until there are integration test in place to validate the logic e2e.

Furthermore this PR is missing unit tests, can you please add?

Thanks for this work!

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.

Add support for fetching application dependencies from HDFS

3 participants