datastax / cassandra-quarkus

An Apache Cassandra(R) extension for Quarkus
Apache License 2.0
40 stars 28 forks source link

JAVA-2754: Integration between Driver reactive API and Quarkus #11

Closed tomekl007 closed 4 years ago

tomekl007 commented 4 years ago

todo:

olim7t commented 4 years ago

I logged JAVA-2792 to provide an easier way to plug 3rd-party reactive types into the mapper.

It feels overkill to me to reimplement a whole mapper processor in this PR, if it's scheduled for obsolescence in a couple of months.

absurdfarce commented 4 years ago

I agree that changing the annotation processor here feels like overkill. It looks like we're hooking in to some implementation details of the base processor impl in java-driver which seems less than desirable from a maintenance/code coupling perspective. Even more importantly (to my eye at least) we're doing all this to avoid telling the user to make a transform in their service impl. Unless I'm missing something (quite possible/probable) we can accomplish everything we need with something like:

@ApplicationScoped
public class FruitReactiveService {
...
  public Multi<Fruit> get(String id) {
    return Wrappers.toMulti(fruitDao.findByIdAsync(id));
  }
}

if we make the Wrappers impl public or maybe:

@ApplicationScoped
public class FruitReactiveService {
...
  public Multi<Fruit> get(String id) {
    return new DefaultMutinyMappedReactiveResultSet<Fruit>(fruitDao.findByIdAsync(id));
  }
}

Note that both the above impls assume the following:

@Dao
public interface FruitDaoReactive {

  @Update
  CompletionStage<Void> updateAsync(Fruit fruitDao);

  @Select
  MappedReactiveResultSet<Fruit> findByIdAsync(String id);
}

You can imagine some other factory methods there to facilitate this type of operation, maybe something like MutinyReactiveResultSetFactory.create(MappedReactiveResultSet) or something along those lines, but you get the point. I don't see any obvious reason why the Dao's have to support returning Mutiny-friendly types directly. And if we drop that requirement big chunks of this PR become unnecessary, the entire mapper-processor module and a Quarkus-specific CqlSession impl most notably.

tomekl007 commented 4 years ago

I agree that changing the annotation processor here feels like overkill. It looks like we're hooking in to some implementation details of the base processor impl in java-driver which seems less than desirable from a maintenance/code coupling perspective. Even more importantly (to my eye at least) we're doing all this to avoid telling the user to make a transform in their service impl. Unless I'm missing something (quite possible/probable) we can accomplish everything we need with something like:

@ApplicationScoped
public class FruitReactiveService {
...
  public Multi<Fruit> get(String id) {
    return Wrappers.toMulti(fruitDao.findByIdAsync(id));
  }
}

if we make the Wrappers impl public or maybe:

@ApplicationScoped
public class FruitReactiveService {
...
  public Multi<Fruit> get(String id) {
    return new DefaultMutinyMappedReactiveResultSet<Fruit>(fruitDao.findByIdAsync(id));
  }
}

Note that both the above impls assume the following:

@Dao
public interface FruitDaoReactive {

  @Update
  CompletionStage<Void> updateAsync(Fruit fruitDao);

  @Select
  MappedReactiveResultSet<Fruit> findByIdAsync(String id);
}

You can imagine some other factory methods there to facilitate this type of operation, maybe something like MutinyReactiveResultSetFactory.create(MappedReactiveResultSet) or something along those lines, but you get the point. I don't see any obvious reason why the Dao's have to support returning Mutiny-friendly types directly. And if we drop that requirement big chunks of this PR become unnecessary, the entire mapper-processor module and a Quarkus-specific CqlSession impl most notably.

Yes, users can achieve that using this approach, but I think that we should make the UX of our framework as good as possible. If we can remove the burden from user to do the conversions manually, we should do it. We are paying the cost of implementing it only once. Otherwise, every user will need to do the conversions itself every time is using our API. Other extensions are following the same pattern. The conversions to Multi are hidden, and only the Multi type is exposed. I think that we should stick to the conventions of other extensions and also make the user's life easier by implementing conversions ourselves.

absurdfarce commented 4 years ago

If we can remove the burden from user to do the conversions manually, we should do it. We are paying the cost of implementing it only once. Otherwise, every user will need to do the conversions itself every time is using our API.

I guess I'd be more sympathetic to this argument if we were talking about something more central to the project. But we're talking about users who (a) want to implement reactive support for (b) their Quarkus apps. Neither of those are enormous populations, so the intersection of them seems... quite small. Which to me looks like we're creating a fairly large amount of framework for something bordering on an edge case. And that framework has to be maintained, tested, reasoned about etc. indefinitely going forward.

Worth noting: the customers will be aware they have to take some steps due to the mismatch of types when they try to get this stuff to compile.. That plus a pretty simple mention in our docs would seem more than adequate to address the issue.

Other extensions are following the same pattern. The conversions to Multi are hidden, and only the Multi type is exposed. I think that we should stick to the conventions of other extensions and also make the user's life easier by implementing conversions ourselves.

Is there another plugin that's doing something directly comparable to what we're talking about here? It would have to be somebody creating a non-trivial amount of engineering to support an alternate use case like this. Also: is there any other plugin that has documentation which says "for this alternate use case you'll need to convert your types like this..." (i.e. something more like what I'm proposing)?

tomekl007 commented 4 years ago

Is there another plugin that's doing something directly comparable to what we're talking about here? It would have to be somebody creating a non-trivial amount of engineering to support an alternate use case like this. Also: is there any other plugin that has documentation which says "for this alternate use case you'll need to convert your types like this..." (i.e. something more like what I'm proposing)?

See the ReactiveMongoClient.

Mutiny types are integrating out of the box with the:

<dependency>
  <groupId>io.quarkus</groupId>
  <artifactId>quarkus-resteasy-mutiny</artifactId>
</dependency>

to create a Reactive REST API.

Neither of those are enormous populations, so the intersection of them seems... quite small.

The reactive way in quarkus is the first choice so I think that users that are using quarkus AND want to create a REST API are actually a majority of users :) From my experience - those frameworks (like spring, quarkus) are mostly used for creating REST APIs so I strongly disagree that this is a small population. If possible, please let's focus on this PR on the implementation instead of questioning the merits of this feature (maybe JIRA, github issues is a better place for that discussion?) because we may get a lot of noise here.

adutra commented 4 years ago

If possible, please let's focus on this PR on the implementation instead of questioning the merits of this feature (maybe JIRA, github issues is a better place for that discussion?) because we may get a lot of noise here.

Yes please let's focus on delivering something here. I agree that the Mutiny subinterfaces and their implementations bring a lot of value, I think this is settled now. We now just need to sort out the mapper problem, and we are currently leaning towards the idea of implementing JAVA-2792 (Support custom mapped results at runtime) first, then we'll revisit this PR and finish it.

tomekl007 commented 4 years ago

https://github.com/datastax/cassandra-quarkus/pull/11#discussion_r433842505 There are two ways:

adutra commented 4 years ago

#11 (comment) There are two ways:

  • using HTML website
  • using CURL what another way would you like to add? Other quickstart projects are following this pattern

It's ok. Closing this debate since it's not going anywhere.