On 26 Mar 2015, at 15:46, Radim Vansa <rvansa(a)redhat.com>
wrote:
On 03/26/2015 03:00 PM, Sanne Grinovero wrote:
> On 26 March 2015 at 13:43, William Burns <mudokonman(a)gmail.com> wrote:
>> On Thu, Mar 26, 2015 at 8:50 AM, Sanne Grinovero <sanne(a)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 :|
>
>
>>> 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> 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> 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>
>>>> 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>
JBoss Performance Team
_______________________________________________
infinispan-dev mailing list
infinispan-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev