[infinispan-dev] Multi get API

Tue Mar 31 14:41:40 EDT 2015

On 31 March 2015 at 16:25, Galder Zamarreño <galder at redhat.com> wrote:
>
>> On 26 Mar 2015, at 15:46, Radim Vansa <rvansa at redhat.com> wrote:
>>
>> On 03/26/2015 03:00 PM, Sanne Grinovero wrote:
>>> On 26 March 2015 at 13:43, William Burns <mudokonman at gmail.com> wrote:
>>>> On Thu, Mar 26, 2015 at 8:50 AM, Sanne Grinovero <sanne at infinispan.org> 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 :|

My initial assumption was that we'd use a varargs parameter to list
the keys to be fetched; in that case getAll() could have been invoked
with zero parameters, and it would look like as it should return all
entries of the grid.

I guess having now a Set for the keys makes the getAll() not too bad.

@William +1 to not implement the future-like behaviour (lazy loaded
HashMap) in the first step. I wasn't asking for that, just thinking
ahead about the merits of returning such a type.

Sanne

>
>>
>>>
>>>
>>>>> 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 at redhat.com> wrote:
>>>>>> On 03/26/2015 10:28 AM, Galder Zamarreño wrote:
>>>>>>> Comments inline:
>>>>>>>
>>>>>>>> On 24 Mar 2015, at 14:22, William Burns <mudokonman at gmail.com> 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 at lists.jboss.org
>>>>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>>>> --
>>>>>>> Galder Zamarreño
>>>>>>> galder at redhat.com
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> infinispan-dev mailing list
>>>>>>> infinispan-dev at lists.jboss.org
>>>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>>>
>>>>>> --
>>>>>> Radim Vansa <rvansa at redhat.com>
>>>>>> JBoss Performance Team
>>>>>>
>>>>>> _______________________________________________
>>>>>> infinispan-dev mailing list
>>>>>> infinispan-dev at lists.jboss.org
>>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>> _______________________________________________
>>>>> infinispan-dev mailing list
>>>>> infinispan-dev at lists.jboss.org
>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>> _______________________________________________
>>>> infinispan-dev mailing list
>>>> infinispan-dev at lists.jboss.org
>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> infinispan-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>>
>> --
>> Radim Vansa <rvansa at redhat.com>
>> JBoss Performance Team
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> infinispan-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
> --
> Galder Zamarreño
> galder at redhat.com
>
>
>
>
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev