Re: [infinispan-dev] Multi get API

> On 26 Mar 2015, at 15:46, Radim Vansa <rvansa(a)redhat.com&gt; wrote: > > On 03/26/2015 03:00 PM, Sanne Grinovero wrote: >> On 26 March 2015 at 13:43, William Burns <mudokonman(a)gmail.com&gt; wrote: >>> On Thu, Mar 26, 2015 at 8:50 AM, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: >>>> In terms of naming, I'd prefer _getMulti_. >>>> My reasoning is it's nice if has a common prefix with _get_ as that >>>> helps someone exploring the API from an IDE, giving better assistance >>>> to people learning. >>>> >>>> getAll is misleading, as is getMany or get getAllOf as they imply I'm >>>> searching for something based on a pattern but you're expecting the >>>> keys and returning tuples key/value. >>> I can understand your point, however since JCache already defined this >>> as getAll I can't see but using that name. >> Ok that makes it win a bit of points, and at least it starts with _get_. > > Was there any discussion regarding this during JSR-107 development? I > wonder why have they chosen such misleading name. Galder? Dunno, never saw it discussed, but I supposed they chose that name cos of Map.putAll I think it's the wrong name, getMany, getMulti are better IMO, regardless of what JCache/java.util.Map do. Also, see what they did with CacheWriter and CacheLoader, they're no symmetrical... and we followed the same pattern in our persistence :|

> >> >> >>>> I like the idea of returning a Map, as you can actually make the map >>>> behave like a Future: the Map instance could be returned right away, >>>> but retrieving of some entries would be blocking until they are >>>> retrieved. You wouldn't need to block on having all entries, just the >>>> ones being actively asked for. In other words, it's better than >>>> returning a Future<Map>. >>>> This should invalidate some of the previous concerns about using Map ? >>> I had thought about this briefly, but didn't give it much though since >>> it makes things quite complicated (interceptor stack and tx context I >>> am looking at you). I would say to do this later if we wanted, since >>> the API doesn't have to be different (other than the fact that people >>> should start possibly expecting CacheExceptions from calling Map.get >>> and other methods). >>> >>>> @William I didn't understand your concern on upper and lower bounds. >>>> The returned types should be coupled to the Cache types; being able to >>> The return type is coupled to the Cache types, this was about the >>> input type defined on the Set. >>> >>>> pass Object might be nice but let's make sure no casting is required >>>> on the client? Could you make some examples of your alternative >>>> proposal? >>> With Set<?> the client would never have to do any casting. >> I'm confused. Not sure which Set you're talking about. And remember >> that if you're being very liberal with the promise of returned types, >> how is the implementation going to handle the guarantee? >> >> For example, what's going to happen if the requested objects don't >> match the user expected type? The API shouldn't make promises it can't >> maintain. > > The cache.get(k1) should return object stored under key k2 such that > > k1.hashCode() == k2.hashCode() && k1.equals(k2) > > Nothing is said about k1's type (though, Object.equals() states that > equals should always form symmetric relation). > The contract was once set for all java collections, let's stick to that. > > Radim > >> >> Thanks, >> Sanne >> >>>> On 26 March 2015 at 10:04, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >>>>> On 03/26/2015 10:28 AM, Galder Zamarreño wrote: >>>>>> Comments inline: >>>>>> >>>>>>> On 24 Mar 2015, at 14:22, William Burns <mudokonman(a)gmail.com&gt; wrote: >>>>>>> >>>>>>> I am nearing completion of the new multi get command [1], allowing for >>>>>>> more efficient retrieval of multiple keys at the same time. Thanks >>>>>>> again to Radim for a lot of the initial work. >>>>>>> >>>>>>> In doing so though I want to make sure I get feedback on how we want >>>>>>> the API to actually look, since this is much more difficult to change >>>>>>> down the line. >>>>>>> >>>>>>> There are a few things in particular I wanted to discuss. >>>>>>> >>>>>>> 1. The actual name of the method. The main suggestions I have seen >>>>>>> are getAll or getMany. I do like the naming of the former, however it >>>>>>> seems a bit misleading (remember API is getAll(Set) since we are >>>>>>> really getting a subset. So I am thinking the possibilities at this >>>>>>> point are getAllOf, getAllFor or getMany. I am leaning maybe towards >>>>>>> the last one (getMany). I am open to any suggestions though. >>>>>> JSR-107 has getAll(Set), which I think it's a bit confusing too naming wise. >>>>>> >>>>>> getMany or getAllOf sound better to me. >>>>>> >>>>>>> 2. What should the return value be for this method. Currently this >>>>>>> returns a Map<K, V> which makes sense when we retain these values in >>>>>>> the local transactional context and is pretty nice and clean for end >>>>>>> users. >>>>>>> >>>>>>> The other idea is to use a streaming API possibly returning an >>>>>>> Iterable<CacheEntry<K, V>>. The streaming return value doesn't seem >>>>>>> as intuitive to me, but opens us up for more efficient usage >>>>>>> especially when there may be a lot of keys passed in (also results can >>>>>>> be processed concurrently while retrieval is still occurring). >>>>>>> >>>>>>> I would lean towards returning the Map<K, V> value, however the next >>>>>>> point could change that. >>>>>>> >>>>>>> 3. Where this method should live. Right now this method is on the >>>>>>> BasicCache interface which is a parent of both Cache (embedded) and >>>>>>> RemoteCache (remote). Unfortunately for remote clients to be as >>>>>>> efficient as possible we really need a Streaming API so that we don't >>>>>>> have multiple copies of data in memory at multiple places at the same >>>>>>> time. For that reason I suggest we use the streaminig API for both or >>>>>>> have a different API for remote vs embedded. >>>>>> Hmmm, I've been thinking about the same thing for Java 8 API (see the proposal wiki), and I think returning a Map limits things. On Java 8, I'm considering switching `evalAll` from returning a Map of CompletableFutures to a Stream... More to come next week. >>>>>> >>>>>> Now, for the current version, I think Map is limiting for both remote and embedded, because even for embedded, retrieving an entry might be a remote operation, so that forces you to wait for all of the entries to be available before you can return the map. Same thing for Remote. >>>>>> >>>>>> What about returning an Iterator? That could be lazy and more suited for the use case? If you need to return a Map, e.g. to implement JCache.getAll(), you can wait for all and construct a map. >>>>> What's the expected usecase? I would expect that users want to just >>>>> retrieve few keys at once, without any fancy lazy >>>>> wait-for-completion-and-do-after stuff. Just imagine that you want to >>>>> pass the two values into another method, Iterator/Stream API wouldn't >>>>> help you with that much. >>>>> >>>>> It seems to me that it is very useful to have multi get with Map<K, V> >>>>> as return type in the API - we don't want to force constructs like: >>>>> >>>>> Iterator<Map.Entry<K, V>> it = cache.multiGet(key1, key2, key3); >>>>> for (; it.hasNext();) { >>>>> Map.Entry<K, V> entry = it.next(); >>>>> if (entry.getKey().equals(key1)) value1 = entry.getValue(); >>>>> else if (entry.getKey().equals(key2)) value2 = entry.getValue(); >>>>> else if (entry.getKey().equals(key3)) value3 = entry.getValue(); >>>>> } >>>>> >>>>> or let them flip it into the map manually (HashMap does not have >>>>> constructor accepting iterator) >>>>> >>>>> Iterator<Map.Entry<K, V>> it = cache.multiGet(key1, key2, key3); >>>>> Map<K, V> map = new HashMap<>(); >>>>> for (; it.hasNext();) { >>>>> Map.Entry<K, V> entry = it.next(); >>>>> map.put(entry.getKey(), entry.getValue()); >>>>> } >>>>> >>>>> >>>>>> Now, whether to have a separate API for this, since we have Java 8 API coming up, I'd not worry too much at this point. >>>>> It seems to me that with 8, we're going to have complicated lazy >>>>> omnipotent API, and several facades simplifying that into Map, JCache or >>>>> whatever. So, let's provide the simplistic API with Map now, and the >>>>> lazy versions can get into the lazy API as soon as we'll see how the >>>>> other methods will look like. >>>>> >>>>> Radim >>>>> >>>>>> Cheers, >>>>>> >>>>>>> Let me know what you guys think. >>>>>>> >>>>>>> Cheers, >>>>>>> >>>>>>> - Will >>>>>>> >>>>>>> [1] https://issues.jboss.org/browse/ISPN-2183 >>>>>>> _______________________________________________ >>>>>>> infinispan-dev mailing list >>>>>>> infinispan-dev(a)lists.jboss.org >>>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>>>> -- >>>>>> Galder Zamarreño >>>>>> galder(a)redhat.com >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> infinispan-dev mailing list >>>>>> infinispan-dev(a)lists.jboss.org >>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>>> >>>>> -- >>>>> Radim Vansa <rvansa(a)redhat.com&gt; >>>>> JBoss Performance Team >>>>> >>>>> _______________________________________________ >>>>> infinispan-dev mailing list >>>>> infinispan-dev(a)lists.jboss.org >>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>> _______________________________________________ >>>> infinispan-dev mailing list >>>> infinispan-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > -- > Radim Vansa <rvansa(a)redhat.com&gt; > JBoss Performance Team > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño galder(a)redhat.com _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev