search

twitter / storehaus

Storehaus is a library that makes it easy to work with asynchronous key value stores
Apache License 2.0
465 stars 92 forks source link

Supporting Scala 12 #341

Closed ghost closed 7 years ago

ghost commented 7 years ago

Hi All,

I dug into this conversion and found 2 spots where I need some feedback: (I am happy to open a PR with what I have to get feedback also, it just doesn't compile ATM).

  1. 2 libraries that don't have overlapping 2.10 and 2.12 versions:

//No 2.10 version that is also for 2.12 for twitter util // Max 2.10: 6.24.0, Min 2.12 6.39.0 val utilVersion = "6.41.0"

//No 2.10 version that is also for 2.12 for finagle //Max 2.10: 6.25.0, Min 2.12: 6.41.0 val finagleVersion = "6.41.0"

The simple way out is dropping 2.10 support. That seems like a big change.

  1. In the upgraded redis finagle client there was a move away from netty3 ChannelBuffer to the 'native' twitter Buf. The redis Store is coded to use ChannelBuffer currently since the current finagle version uses that for redis.

https://github.com/twitter/finagle/blob/develop/finagle-redis/src/main/scala/com/twitter/finagle/redis/Client.scala#L159

This introduces a good number of conversions to and from Buf <-> netty.ChannelBuffer. I am happy to work though the conversions as part of the PR. My questions is: Do you think converting Buf <-> netty.ChannelBuffer is the right path here?

piyushnarang commented 7 years ago

I think for twitter util 2.10 vs 2.12, I believe they're not publishing twitter util for 2.10 anymore as it has been EOL for sometime. In bijection we're using an older version for the 2.10 build and a different one for the 2.11 and 2.12 - https://github.com/twitter/bijection/blob/develop/build.sbt#L14. When we discussed this a few weeks back, I don't think anyone had strong objections to dropping support for 2.10. As we were able to work around it, we kept the support going.

ghost commented 7 years ago

OK, I will try out the version fallback strategy and see if it works. Thanks for the suggestion.

My guess is since the redis API changed so much I won't be able to do the fallback for finagle but I will give it a try.

ghost commented 7 years ago

Thinking through how storhaus works the conversion between Buf and ChannelBuffer is handled really well by the store concept anyway. The in-framework conversion is not required as any user that needs a ChannelBuffer can easily continue to use it after upgrading. I am going to just swap all the types to Buf.

johnynek commented 7 years ago

Actually, can you minimize the change possible? Each incompatibility is a tax on consumers. If we really can't keep ChannelBuffer around, okay, but what might be a small change for someone with only a few uses of the library can become a show stopper for someone with 400 instances.

ghost commented 7 years ago

Good point. I will get this change set working first then revisit how to convert a RedisStore[Buf, Buf] to RedisStore[ChannelBuffer, ChannelBuffer] in a clean way. I think it should be pretty easy given the abstractions on stores.