ISPN-9922 New API modules #6656

karesti · 2019-02-04T17:42:28Z

https://issues.jboss.org/browse/ISPN-9922

This is the first step. These modules are new, actual code should not be impacted by this work.
This PR contains:

New API module
New hotrod-client reactive module
Reactive key-value store with simple methods

Needs to be done - maybe not now but in a near future PR because this is very new

Javadocs
Documentation

This PR is part of an umbrella issue

karesti · 2019-02-05T12:05:21Z

failures not related

karesti · 2019-02-05T20:59:29Z

This PR focus is adding the two new modules. APIs are not definitive at all. This will be treated in other pull requests

karesti · 2019-02-06T10:52:26Z

The failure is not related

karesti · 2019-02-07T15:42:32Z

I changed the base branch for tis PR

* New API module * New hotrod-client reactive module * Reactive key-value store with simple methods

galderz · 2019-02-12T11:04:07Z

I've said this @karesti before but I really want to lay it down here so that others can read:

I'd really like to see a plan on how we plan to evolve APIs over time. What recommendations should developers follow when they want to add/change/remove API methods? At which point they should come up with new interfaces and how do users migrate from one to the other? When should we create new modules? When can we reuse existing API modules?

The answers to these questions should take end user into consideration to see what makes their life easy when upgrading or changing things.

All this needs to be sketched out before deciding which APIs and which methods to include, because you're never going to get that right. So, the best way to future protect yourself against deciding between api A or B, or whether to include this or that method, is to have a clear plan on how to evolve things.

Not doing this when we started Infinispan is by far the biggest mistake we did API wise. I don't want us to repeat the same mistake again.

karesti · 2019-02-12T12:49:03Z

I've said this @karesti before but I really want to lay it down here so that others can read:

I'd really like to see a plan on how we plan to evolve APIs over time. What recommendations should developers follow when they want to add/change/remove API methods? At which point they should come up with new interfaces and how do users migrate from one to the other? When should we create new modules? When can we reuse existing API modules?

The answers to these questions should take end user into consideration to see what makes their life easy when upgrading or changing things.

All this needs to be sketched out before deciding which APIs and which methods to include, because you're never going to get that right. So, the best way to future protect yourself against deciding between api A or B, or whether to include this or that method, is to have a clear plan on how to evolve things.

Not doing this when we started Infinispan is by far the biggest mistake we did API wise. I don't want us to repeat the same mistake again.

ok.

Which concrete output would you like to see produced? A readme document?

Here is the issue related to the task: https://issues.jboss.org/browse/ISPN-9966

galderz · 2019-02-15T09:09:53Z

Which concrete output would you like to see produced? A readme document?

A section in the contribution guide. I also would like to see a separate repo with examples where we show contributors potential scenarios. E.g.

If you want to phase out a return type (e.g. reactivestreams APIs and use Java 9+ reactivestreams types), then do this.
If you want to add a new API do this.
If you want to add a new method, do this... (make sure it has default modifier)

galderz · 2019-02-15T09:12:33Z

It's important to do this sooner rather than later so that we know the foundations that put in place for new API is as future proof as possible (at the time of writing). I believe that unless we sit down and write these examples we'll miss things that might make our new API infrastructure not as future proof as we'd like and eventually we'll have to revisit parts in some kind of breaking change.

galderz · 2019-02-15T09:26:08Z

Btw, the moment we're based on Java 11 we should be able to use JDK's reactive streams classes.

galderz · 2019-02-15T09:26:48Z

Dealing with that would have an example when a new version of the API needs to be exposed.

galderz · 2019-02-15T09:41:39Z

Related to the topic of other APIs, are you expecting all APIs to live in the api/ module? Not sure this is a good idea from the PoV of isolating APIs and only depending on what are need.

galderz

I'm happy with the existing KeyValueStore API and I'm happy with the use of CompletionStage+Publisher. One thing I've noticed is that in Clement's document he specifies that CompletableFuture should be used as return type and here CompletionStage is used. I think the latter is better suited, but wondering why Clement mentions CompletableFuture in his reactive API doc. @karesti Can you double check with him?

I'm lacking some vision/design on how the rest of APIs would fit here, where would they go...etc. This is important from the POV of deciding whether the location of KeyValueStore API is the right one or not.

karesti · 2019-02-18T11:27:23Z

@galderz I've sent you a link with another doc

karesti · 2019-02-18T11:31:38Z

Related to the topic of other APIs, are you expecting all APIs to live in the api/ module? Not sure this is a good idea from the PoV of isolating APIs and only depending on what are need.

This is one of the things we talked about in the F2F. This is one of the strategies, we could not decide what to do without being able to see how it would potentially look. I decided to implement a first thing based on the idea we will have api jar + impl jar to use a particular API.

karesti · 2019-02-18T11:32:26Z

@galderz This first work could be experimental and just for us before promoting it to a new official way of doing things from now on in infinispan

ryanemerson · 2019-02-19T10:52:18Z

api/src/main/java/org/infinispan/api/collections/reactive/KeyValueStore.java

+ * A Reactive Key Value Store provides a highly concurrent and distributed data structure, non blocking and using
+ * reactive streams.
+ * <p>
+ * TODO: Add more javadoc and examples


Is this being added as part of this PR?

No. This PR is more about having 2 new modules

ryanemerson · 2019-02-19T10:53:06Z

...-client-reactive/src/main/java/org/infinispan/api/client/impl/ClientConfigurationLoader.java

+import org.infinispan.client.hotrod.configuration.ConfigurationBuilder;
+
+/**
+ *


Docs needed

ryanemerson · 2019-02-19T10:54:45Z

...otrod-client-reactive/src/main/java/org/infinispan/api/client/impl/InfinispanClientImpl.java

+
+   @Override
+   public <K, V> KeyValueStore<K, V> getKeyValueStore(String name) {
+      RemoteCache<K, V> cache = cacheManager.getCache(name, false);


Won't this throw a NPE if the ClientConfig passed to the constructor is not a instanceof ClientConfigurationLoader.ConfigurationWrapper?

probably. this is not even tested yet, just a quick thing. I think this PR is causing more confusion than anything

ryanemerson · 2019-02-19T10:55:26Z

...otrod-client-reactive/src/main/java/org/infinispan/api/client/impl/InfinispanClientImpl.java

+   }
+
+   /* TODO: Remove, visible for test now */
+   public void setCacheManager(RemoteCacheManager cacheManager) {


can be package-private if just for tests.

ryanemerson · 2019-02-19T10:59:45Z

...od-client-reactive/src/test/java/org/infinispan/api/client/impl/KeyValueStoreSimpleTest.java

+
+      await(store.putMany(Flowable.fromIterable(entries)));
+
+//      store.findContinuously();


ryanemerson · 2019-02-19T11:02:04Z

api/pom.xml

+    <description>Infinispan Api</description>
+
+    <properties>
+        <version.reactivestreams>1.0.1</version.reactivestreams>


I think this should be set in the build-configuration/pom.xml or pom.xml?

ryanemerson · 2019-02-19T11:02:42Z

api/src/main/java/org/infinispan/api/InfinispanClient.java

+ */
+public class InfinispanClient {
+
+   //TODO: Decide how to test and how to make this work


ryanemerson · 2019-02-19T11:03:03Z

api/src/main/java/org/infinispan/api/collections/reactive/CachePublisher.java

+import org.reactivestreams.Publisher;
+
+/**
+ * I -> Infinispan xD


This might need to be a little more informative 😄

ryanemerson · 2019-02-19T11:06:18Z

api/src/main/java/org/infinispan/api/collections/reactive/KeyValueEntry.java

+   private final V value;
+
+   public KeyValueEntry(K key, V value) {
+


This is basically the same as org.infinispan.util.KeyValuePair .. if we're going to use this a lot, we should probably deprecate KeyValuePair.

Also, I can see this class being useful outside of just reactive contexts. Maybe we should put in a more generic package, such as org.infinispan.api.collections.util or just org.infinispan.api.collections, WDYT?

galderz · 2019-02-19T12:05:02Z

This is one of the things we talked about in the F2F. This is one of the strategies, we could not decide what to do without being able to see how it would potentially look. I decided to implement a first thing based on the idea we will have api jar + impl jar to use a particular API.

Fair enough...

However, I see two problems I found with this approach are:

How does one add a new API? Currently you'd have to modify the source API module and add your API to Infinispan object. This works if we're not interested in promoting APIs created by external users, which includes not only end users, but also consuming projects (e.g. OGM).
Say your API module exposes a RxJava APIs A.B.C and the user wants to use the plain KV API and it uses Rx Java APIs X.Y.Z. Such user might have issues mixing 3rd party libraries that the API brings indirectly (even though the user does not actually use them) with APIs that it uses.

@karesti You should write down the decisions/options you've made WRT API, write down the reasons why you choose that, and you should also track the potential problems. Only this way we can get a full picture of why things are the way they are and the problems we've decided not to solve.

karesti · 2019-02-19T12:17:29Z

This is one of the things we talked about in the F2F. This is one of the strategies, we could not decide what to do without being able to see how it would potentially look. I decided to implement a first thing based on the idea we will have api jar + impl jar to use a particular API.

Fair enough...

However, I see two problems I found with this approach are:

How does one add a new API? Currently you'd have to modify the source API module and add your API to Infinispan object. This works if we're not interested in promoting APIs created by external users, which includes not only end users, but also consuming projects (e.g. OGM).

Say your API module exposes a RxJava APIs A.B.C and the user wants to use the plain KV API and it uses Rx Java APIs X.Y.Z. Such user might have issues mixing 3rd party libraries that the API brings indirectly (even though the user does not actually use them) with APIs that it uses.

@karesti You should write down the decisions/options you've made WRT API, write down the reasons why you choose that, and you should also track the potential problems. Only this way we can get a full picture of why things are the way they are and the problems we've decided not to solve.

I wrote this down in the API manifesto. This PR is a previous one

karesti · 2019-02-19T12:21:43Z

closing in favor for #6690 for the first steps in the code, and the API manifesto #6678

danberindei · 2019-02-27T15:55:50Z

I just saw @galderz's comment so I'm adding a late reply here.

How does one add a new API? Currently you'd have to modify the source API module and add your API to Infinispan object. This works if we're not interested in promoting APIs created by external users, which includes not only end users, but also consuming projects (e.g. OGM).

-1 from me to promote APIs created by external users, it would complicate things, and some (most?) consuming projects will want to hide the underlying caches/key-value stores anyway.

Say your API module exposes a RxJava APIs A.B.C and the user wants to use the plain KV API and it uses Rx Java APIs X.Y.Z. Such user might have issues mixing 3rd party libraries that the API brings indirectly (even though the user does not actually use them) with APIs that it uses.

Clement's reactive strategy proposal suggests exposing in our APIs only the Publisher from reactive streams. So the API module should only depend on reactive-streams and nothing else.

One thing I've noticed is that in Clement's document he specifies that CompletableFuture should be used as return type and here CompletionStage is used.

The newest version of the doc uses CompletionStage.

Btw, the moment we're based on Java 11 we should be able to use JDK's reactive streams classes.

The doc also suggests getPublisher() for the JDK version getRsPublisher() for the reactive-streams version. IDK if it would require multi-version jars or not, but I'm sure we can support Flow.Publisher right now.

karesti · 2019-02-28T11:07:27Z

@danberindei thank you! you are welcome to check the other two PR I've mentioned above 💃 🕺

karesti force-pushed the ISPN-9922 branch 3 times, most recently from 9bdbff7 to e1b3818 Compare February 5, 2019 09:30

karesti changed the title ~~ISPN-9922 New Reactive API~~ ISPN-9922 New API modules Feb 5, 2019

karesti force-pushed the ISPN-9922 branch from e1b3818 to cc27244 Compare February 5, 2019 20:57

karesti added this to the 10.0.0.Beta1 milestone Feb 5, 2019

karesti requested a review from galderz February 5, 2019 21:00

karesti force-pushed the ISPN-9922 branch from cc27244 to 8681533 Compare February 5, 2019 21:04

karesti force-pushed the ISPN-9922 branch from 8681533 to 9cff67e Compare February 6, 2019 12:07

karesti changed the base branch from master to new-api February 7, 2019 15:41

karesti force-pushed the ISPN-9922 branch from 9cff67e to 719ce04 Compare February 7, 2019 15:42

karesti added 2 commits February 8, 2019 18:01

ISPN-9922 First steps towards new API

1836519

* New API module * New hotrod-client reactive module * Reactive key-value store with simple methods

ISPN-9922 put search semantics in KeyValueStore API

6efb098

karesti force-pushed the ISPN-9922 branch from 719ce04 to 6efb098 Compare February 8, 2019 17:09

galderz reviewed Feb 15, 2019

View reviewed changes

ryanemerson reviewed Feb 19, 2019

View reviewed changes

karesti closed this Feb 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISPN-9922 New API modules #6656

ISPN-9922 New API modules #6656

karesti commented Feb 4, 2019 •

edited

karesti commented Feb 5, 2019

karesti commented Feb 5, 2019

karesti commented Feb 6, 2019

karesti commented Feb 7, 2019

galderz commented Feb 12, 2019

karesti commented Feb 12, 2019

galderz commented Feb 15, 2019

galderz commented Feb 15, 2019

galderz commented Feb 15, 2019

galderz commented Feb 15, 2019

galderz commented Feb 15, 2019

galderz left a comment

karesti commented Feb 18, 2019 •

edited

karesti commented Feb 18, 2019

karesti commented Feb 18, 2019

ryanemerson Feb 19, 2019

karesti Feb 19, 2019

ryanemerson Feb 19, 2019

ryanemerson Feb 19, 2019

karesti Feb 19, 2019

ryanemerson Feb 19, 2019

ryanemerson Feb 19, 2019

ryanemerson Feb 19, 2019

ryanemerson Feb 19, 2019

ryanemerson Feb 19, 2019

ryanemerson Feb 19, 2019

galderz commented Feb 19, 2019

karesti commented Feb 19, 2019

karesti commented Feb 19, 2019

danberindei commented Feb 27, 2019 •

edited

karesti commented Feb 28, 2019


		await(store.putMany(Flowable.fromIterable(entries)));

		// store.findContinuously();

		private final V value;

		public KeyValueEntry(K key, V value) {

ISPN-9922 New API modules #6656

ISPN-9922 New API modules #6656

Conversation

karesti commented Feb 4, 2019 • edited

karesti commented Feb 5, 2019

karesti commented Feb 5, 2019

karesti commented Feb 6, 2019

karesti commented Feb 7, 2019

galderz commented Feb 12, 2019

karesti commented Feb 12, 2019

galderz commented Feb 15, 2019

galderz commented Feb 15, 2019

galderz commented Feb 15, 2019

galderz commented Feb 15, 2019

galderz commented Feb 15, 2019

galderz left a comment

Choose a reason for hiding this comment

karesti commented Feb 18, 2019 • edited

karesti commented Feb 18, 2019

karesti commented Feb 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galderz commented Feb 19, 2019

karesti commented Feb 19, 2019

karesti commented Feb 19, 2019

danberindei commented Feb 27, 2019 • edited

karesti commented Feb 28, 2019

karesti commented Feb 4, 2019 •

edited

karesti commented Feb 18, 2019 •

edited

danberindei commented Feb 27, 2019 •

edited