Use "aws_access_key_id" and "aws_secret_access_key" for reads, not just unload

[koeninger]

No description provided.

[codecov-io]

Current coverage is 84.98%

Merging #32 into master will increase coverage by +0.29% as of 9ebb0b9

@@            master     #32   diff @@
======================================
  Files           10      10       
  Stmts          307     313     +6
  Branches        67      67       
  Methods          0       0       
======================================
+ Hit            260     266     +6
  Partial          0       0       
  Missed          47      47       

Review entire Coverage Diff

Powered by Codecov

[JoshRosen]

Another test coverage issue :(

[koeninger]

Yeah, there's a separate earlier PR just for that one line... got pulled into this branch by accident as I'm trying to get things working

[marmbrus]

/cc @kai-zeng who has a WIP branch with a similar fix.

[JoshRosen]

Hey @koeninger, do you have a regression test for this? Now that we have the infra from #41 in place I think it would be a good idea to test this.

[JoshRosen]

Hmm, I'm wondering why the fact that I'm missing this fix in my end-to-end tests has not led to a test failure yet.

[eduardoramirez]

@JoshRosen I came across this issue as well. It worked fine when running the job an EC2 instance that was allowed to read/write from S3. However, when running the job locally, I needed to set the hadoop configuration with my keys.

[JoshRosen]

@eduardoramirez @koeninger, given that both of you have reported this issue and fix I'm inclined to merge this now and figure out the regression tests later. I'm still puzzled over why I haven't been able to get this to fail in Travis, especially given that I purposely picked environment variables that don't match the ones that AWS expects. I'll take a closer look at our S3 bucket permissions tomorrow to see if that could explain the problem.

[JoshRosen]

I think I've spotted the problem: during test setup, we end up setting some Hadoop Configuration access key properties on the global SparkContext.hadoopConfiguration. I'm going to remove those lines, verify that the tests fail, then will rebase and merge this fix.

[JoshRosen]

I've opened #55 to bring this up to date and to fix the tests so that they're capable of detecting the bug being fixed here.