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

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

Open
marekzebrowski opened this issue Jun 17, 2016 · 9 comments
Open

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

marekzebrowski opened this issue Jun 17, 2016 · 9 comments

Comments

@marekzebrowski
Copy link

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?

@mateor
Copy link
Contributor

mateor commented Jun 18, 2016

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
Copy link
Contributor

mateor commented Jun 30, 2016

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.

@marekzebrowski
Copy link
Author

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
Copy link
Author

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

@jvandew
Copy link
Contributor

jvandew commented Feb 3, 2017

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.

@marekzebrowski
Copy link
Author

marekzebrowski commented Feb 5, 2017

@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 mongodb/mongo-scala-driver@f4a1717 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.

@jvandew
Copy link
Contributor

jvandew commented Feb 6, 2017

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.

@marekzebrowski
Copy link
Author

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
Copy link
Author

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

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