Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/8880#issuecomment-158232589
Hi @winningsix , I left a few comments above, but a few other observations:
- Please also update the PR description (the first comment); it still
refers to the previous code.
- Because you're stashing the encryption key in the Hadoop credentials,
this will only work with YARN. Personally that's fine with me to start with,
but the code should error out if not running on YARN and shuffle encryption is
turned on.
- The tests don't seem to really be testing the code you're adding; it
would be nice to add some test that actually exercises Spark's shuffle code
with encryption enabled.
An alternative to the credentials approach would be what's done for the
authentication secret; it uses the credentials object on YARN but an
environment variable on other managers. Not sure how easy it would be to do
that, but can probably be done as a separate change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
