search

spark-redshift-community / spark-redshift

Performant Redshift data source for Apache Spark
Apache License 2.0
136 stars 62 forks source link

spark-3.0.1 compatible #77

Closed vanhoale closed 4 years ago

vanhoale commented 4 years ago

how does this compare to #70? (just making sure I understand the differences 😄 )

I think this is a subset of that work (except the spark v3.0.0 vs 3.0.1). This is a much smaller PR, so is ok to be a subset.

Hi @jsleight thanks for looking at this PR and for pointing me to the #70 . My PR is just a simple spark version upgrade, it may help to resolve the #76 quickly, otherwise, we can wait for #70 release

vanhoale commented 4 years ago

hi @lucagiovagnoli so we can't test the same code base with 2 spark api versions

codecov-io commented 4 years ago

Codecov Report

Merging #77 into master will not change coverage. The diff coverage is 90.90%.

@@           Coverage Diff           @@
##           master      #77   +/-   ##
=======================================
  Coverage   74.09%   74.09%           
=======================================
  Files          15       16    +1     
  Lines         772      772           
  Branches      106       94   -12     
=======================================
  Hits          572      572           
  Misses        200      200
lucagiovagnoli commented 4 years ago

Looks good! What kind of testing have you performed ?

Gentle bump on this question. It seems to me that moving to spark3 required an awful small amount of code and my gut feeling tells me that something is wrong. Do all tests pass ? I will also try running this against our internal integration suite

vanhoale commented 4 years ago

Looks good! What kind of testing have you performed ?

Gentle bump on this question. It seems to me that moving to spark3 required an awful small amount of code and my gut feeling tells me that something is wrong. Do all tests pass ? I will also try running this against our internal integration suite

yes, it seems all the integration suites completed successfully when I did run sbt it:test locally. Does travis CI do the same?

[info] IAMIntegrationSuite:
[info] - roundtrip save and load !!! IGNORED !!!
[info] - load fails if IAM role cannot be assumed !!! IGNORED !!!
[info] PostgresDriverIntegrationSuite:
[info] - postgresql driver takes precedence for jdbc:postgresql:// URIs !!! IGNORED !!!
[info] - roundtrip save and load !!! IGNORED !!!
20/10/07 18:05:57 INFO ShutdownHookManager: Shutdown hook called
20/10/07 18:05:57 INFO ShutdownHookManager: Deleting directory /private/var/folders/h9/w3kwqgt52vd6044pmp_kglx817hfbk/T/spark-db2c39fe-aa22-43e7-8f30-5b65fa1ba91e
[info] Run completed in 44 minutes, 44 seconds.
[info] Total number of tests run: 48
[info] Suites: completed 11, aborted 0
[info] Tests: succeeded 48, failed 0, canceled 0, ignored 7, pending 0
[info] All tests passed.
[success] Total time: 2688 s, completed Oct 7, 2020 6:05:57 PM
lucagiovagnoli commented 4 years ago

Travis CI doesn't run it:test iirc but I've also just manually verified internally that it:test works. So this PR looks good to me. Before merging it in and publishing to nexus, @jsleight, @lydian how do you feel about 4.2.1 vs 5.0.0 ?

jsleight commented 4 years ago

Travis CI doesn't run it:test iirc but I've also just manually verified internally that it:test works. So this PR looks good to me. Before merging it in and publishing to nexus, @jsleight, @lydian how do you feel about 4.2.1 vs 5.0.0 ?

Minor version update imo, so 4.2.0. Our public API is unchanged, and this faq on semver page suggests that updates to package dependencies should be minor or patch in these cases. I view this as a rather significant dependency update, so minor version bump seems correct?

lucagiovagnoli commented 4 years ago

This is released on sonatype

vanhoale commented 4 years ago

This is released on sonatype

Thanks @lucagiovagnoli