Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document deprecated and breaking changes for 0.8.0 release #2076

Closed
1 task done
regadas opened this issue Jul 24, 2019 · 8 comments
Closed
1 task done

Document deprecated and breaking changes for 0.8.0 release #2076

regadas opened this issue Jul 24, 2019 · 8 comments
Assignees
Milestone

Comments

@regadas
Copy link
Contributor

regadas commented Jul 24, 2019

Things missing from migration doc:

@regadas regadas self-assigned this Jul 24, 2019
@regadas regadas added the P1 label Jul 24, 2019
@regadas regadas added this to the 0.8.0 milestone Jul 24, 2019
@brianmartin
Copy link
Contributor

I am giving 0.8.0 a spin. It would be really helpful if all the deprecations had the suggested new thing.

I can definitely imagine folks throwing up their hands when they encounter:

[warn] /Users/brianm/src/ghe/ml/tfrecord-components/tfrecord-components/src/main/scala/com/spotify/ml/tfrecord_components/converters/bq/BqToTfRe
cordJob.scala:56:8: method close in class ScioContext is deprecated (since Scio 0.8.0): this method will be removed in next scio version
[warn]     sc.close()
[warn]        ^

sc.run() is the new thing, right?

@regadas
Copy link
Contributor Author

regadas commented Jul 25, 2019

@brianmartin yup that's the one!

@jto
Copy link
Contributor

jto commented Nov 11, 2019

@regadas regarding the breaking changes documented in https://spotify.github.io/scio/migrations/v0.8.0.html#scala-concurrent-future-removed-from-scioios, do you think we could provide a bunch of implicit convertions that would provide a "compatibility" layer to avoid breaking changes ?

Let say we provide the following implicit conversions

@deprecated(since = "0.8.0", "ScioIO#read does not return an Future anymore. Please see the following documentation: xxx") 
implicit def toFutureTap[T](t: Tap[T]): Future[Tap[T]] = ???

@deprecated(since = "0.8.0", "ScioIO#write does not return an Future anymore. Please see the following documentation: xxx") 
implicit def toClosedTap[T](t: Future[Tap[T]]):  ClosedTap[T] = ???

we could limit the impact of upgrading from 0.7 to 0.8 while still moving to a cleaner API. Is there anything else we should implicitly convert ?

I think to scalafix rules I implemented are not really good enough so this could be a good alternative.

wdyt ?

@jto
Copy link
Contributor

jto commented Nov 11, 2019

Looking at the codebase at Spotify, it seems that the removal of AsyncLookupDoFn and AsyncLookupDoFn.Try will affect a lot of pipelines. We should certainly have some compatibility convertions for this too. It seems that having a type alias may be enough to avoid the issue.

@jto
Copy link
Contributor

jto commented Nov 12, 2019

@regadas I started working on a PR: #2401

@regadas
Copy link
Contributor Author

regadas commented Nov 12, 2019

@jto yeah sounds like a plan! do you think it's feasible to have them under a compat package? that way will be easier to track it down and remove it

@jto
Copy link
Contributor

jto commented Nov 12, 2019

We could for most of them but I'd rather have something that makes upgrading easy and hopefully requires minimal manual intervention. Having a different package name would defeat that.

@regadas
Copy link
Contributor Author

regadas commented Jan 9, 2020

0.8.0 is out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants