fsqio icon indicating copy to clipboard operation
fsqio copied to clipboard

non-blocking mongo support for rogue - are you interested ?

Open marekzebrowski opened this issue 8 years ago • 9 comments

Hi, I developed a for of rogue that uses mongo-drivers in versions 3.2.x and adds async operation support to rogue https://github.com/sgrouples/rogue I was not aware of fsqio monorepo, so I presumed that rogue development is dead. Are you interesting in such contribution?

marekzebrowski avatar Jun 17 '16 09:06 marekzebrowski

Hi!

It is always an exciting day when someone drops by with a potential contribution. I am commenting to let you know that we have seen your issue - I will bring it up for discussion at our server meeting this week and we can pick this up from there. I appreciate the offer and will reply here before the end of the week.

mateor avatar Jun 18 '16 18:06 mateor

Okay, I wanted to respond to you now that we have had a chance to discuss it internally.

Firstly - the feature is great and does something that we absolutely are interested in. People were pretty blown away to see it show up in a Github issue.

Fsq.io is a somewhat unique repo - it is a strict subset of our internal codebase. This means that it has Rogue exactly as we use it internally - which is great for potential OSS contributions. But it also brings some challenges since it is a subset of our internal usage. So with a feature this large there is a non-trivial amount of work to be done to verify that the new feature covers all of our business-critical usage.

So this is basically a status update to say that, yes, we are very interested in the contribution. But we have to take some time and make sure it is feasible for us to consume it at this moment.

One of our engineers has an interest in this area and has volunteered to give your feature a deeper look. I will be sure to update you once they have had time to do so.

mateor avatar Jun 30 '16 19:06 mateor

Nice to hear! I started to re-apply my work to the version in fsqio, but it is not yet ready to be merged. I have found it non-trivial though - as my version requires patched lift and fsqio heavily depends on spindle that I don't want to pull. Anyway - I'm sure that with a bit of work we can move forward. My ultimate goal is to have lift-free version of rogue, with async operations and full case-class support (via salat or shapeless maybe), but I'm not yet there. I am aware that our goals are not exactly the same, but I think that the room for cooperations is large enough that we all can benefit.

marekzebrowski avatar Jul 01 '16 07:07 marekzebrowski

Update: I ported async changes to a rogue derived from fsqio Unfortunately it is not a patch over monorepo for following reasons:

  1. fsqio version introduces tight coupling with spindle via spindle imports in MongoIndexChecker
  2. pants build - as we use sbt, I just moved it to it But otherwise it is code from fsqio + async + minor shifts

See changes here: https://github.com/sgrouples/rogue-fsqio

marekzebrowski avatar Jul 06 '16 07:07 marekzebrowski

Happy New Year!

First off, apologies for the delay in getting back to you. We only managed to get our codebase upgraded to version 3 of the mongo driver in November and had (still have actually) some fallout to deal with from that change.

I did get a chance to dig into your code, and you definitely have a great implementation there (and tests!). Ultimately though, I think it makes the most sense for us to pass on this for a couple reasons.

You are making great strides with case class models and async support, but as you mentioned our ultimate goals are a bit different. Unfortunately, we're going to be stuck supporting both lift and spindle models for the foreseeable future, and while it's my intention to move us to the async client, we will not be getting rid of the blocking client anytime soon either. Our stack is also very heavily centered around twitter-util's concurrent library instead of scala.concurrent. I think the solution you have here is much more useful for consumers outside Foursquare in that regard than it is for us internally.

Ultimately then, I don't think it makes sense for us to merge this into fsq.io just because we would have to re-implement much of your work for our own stack anyway. And it definitely makes sense for you to continue owning your case class model implementation; it will get much more love being outside of our codebase.

That said, there are couple things we could do here:

  1. I've spec'ed out the change internally to implement the async client and update the blocking client to the new driver api, with the goal of being able to share most of their code and support both lift and spindle with minimal extra effort. It is being designed in a way where the end result will be something you could easily build from if you wish. I personally think that is the best solution for everyone in the long run, although I realize unforking is quite a bit of effort on your part.

  2. We made a change a couple months ago to pull the spindle dependency out of rogue. If you did wish to unfork we could likely do something similar and ship lift support as a separate jar (I believe we actually used to do this historically).

  3. For all it does, Rogue is built for our own internal needs which don't always match the broader scala community (as you well know :) ). I think you have a great solution with case class model support and the scala.concurrent library that others would be interested in, and I would love to point to your project somewhere in the rogue/fsq.io readmes if you're ok with that.

Let us know if you have any thoughts/questions. As @mateor mentioned, we are definitely excited and really appreciate the fact you brought this to us, even if it isn't something we end up consuming.

jvandew avatar Feb 03 '17 21:02 jvandew

@jvandew thanks for your response. I understand your motives and I agree that your needs does not represent all possible use-cases, and as a in-house project it first needs to serve your needs, so decision not to merge that code is perfectly understandable. I'm grateful that you shared Rogue DSL and implementation as Open Source, so we could benefit from it. As for the future - we would benefit a bit from removing spindle, but it is not a major burden right now, it was rather easy to remove. Our current goals for rogue-cc as we call it are:

  1. Support case classes in best possible way. This is our default
  2. Keep proven rogue DSL
  3. provide fully-async API
  4. for some time - keep backwards compatibility with Lift.

Obviously those are not the same as yours. As for 1.) mongo-scala-driver will get macro-based case class mapper https://github.com/mongodb/mongo-scala-driver/commit/f4a17179d6507c3b7556eab824d955b8c7fdb7c2 in future release, so maybe we will be able to drop our shapeless version, but that remains to be seen. For time being we still need to support legacy code with LiftRecord.

marekzebrowski avatar Feb 05 '17 18:02 marekzebrowski

There has been some interest from others (see #36) to have support for the scala driver so I am certainly interested to see where you go with that. It isn't something we're considering internally for now, but definitely worth thinking about for the future.

I will circle back around in a few weeks once I've had a chance to spend some time on this and see where we stand. In the meantime, let me know if you would be comfortable with us linking to your project in the readme somewhere.

jvandew avatar Feb 06 '17 19:02 jvandew

Feel free to link our project https://github.com/sgrouples/rogue-fsqio Unfortunately it is not straight forward to use at this point, as we don't publish artifacts and require own version of lift.

marekzebrowski avatar Feb 06 '17 20:02 marekzebrowski

there is also ongoing effort to make async version of rogue available in lift 3.1 https://github.com/lift/framework/pull/1825 so migration to Scala 2.12 (big issue for us) will be easier

marekzebrowski avatar Feb 06 '17 20:02 marekzebrowski