Hi, keySet(), values(), entrySet() do not seem to be implemented at the moment: public Set<K> keySet() { throw new UnsupportedOperationException("TODO implement me"); // TODO implement me } public Collection<V> values() { throw new UnsupportedOperationException("TODO implement me"); // TODO implement me } public Set<Map.Entry<K, V>> entrySet() { throw new UnsupportedOperationException("TODO implement me"); // TODO implement me } We should be implementing these methods, shouldn't we? Users might want to iterate the map for example to print the contents or to verify it contains what they put on it...etc. I can't see a JIRA for it, so I'll open one so that this gets implemented for Beta1 unless someone has any objections. On IRC, Mircea said it might very costly, specially for dist, but we still need it regardless, otherwise we're not fully implementing the ConcurrentMap interface and these are commonly used methods.

Regards,

Galder Zamarreno wrote: > Hi, > > keySet(), values(), entrySet() do not seem to be implemented at the > moment: > > public Set<K> keySet() { > throw new UnsupportedOperationException("TODO implement me"); // > TODO implement me > } > > public Collection<V> values() { > throw new UnsupportedOperationException("TODO implement me"); // > TODO implement me > } > > public Set<Map.Entry<K, V>> entrySet() { > throw new UnsupportedOperationException("TODO implement me"); // > TODO implement me > } > > We should be implementing these methods, shouldn't we? Users might > want to iterate the map for example to print the contents or to verify > it contains what they put on it...etc. > > I can't see a JIRA for it, so I'll open one so that this gets > implemented for Beta1 unless someone has any objections. > > On IRC, Mircea said it might very costly, specially for dist, but we > still need it regardless, otherwise we're not fully implementing the > ConcurrentMap interface and these are commonly used methods. for DIST these might be implemented as follows: -the node broadcasts a 'GetKeys' command to all others nodes in the cluster - on remote nodes locks are acquired for all keys (not sure this mandatory though) - each node responds with the set of keys - calling node puts them together and returns them to the user for entrySet this is even more costly, as all cluster state shall be transfered to the calling node. Now, what worries me is the fact that users might overuse these methods, without being aware of the performance consequences. A possible way to enforce the users to acknowledge that these operations are costly, is to disable them by default and allow a config element to enable them.

> > Regards,

On 4 Jun 2009, at 10:15, Galder Zamarreno wrote: > > > Mircea Markus wrote: >> Galder Zamarreno wrote: >>> Hi, >>> >>> keySet(), values(), entrySet() do not seem to be implemented at the >>> moment: >>> >>> public Set<K> keySet() { >>> throw new UnsupportedOperationException("TODO implement me"); >>> // TODO implement me >>> } >>> >>> public Collection<V> values() { >>> throw new UnsupportedOperationException("TODO implement me"); >>> // TODO implement me >>> } >>> >>> public Set<Map.Entry<K, V>> entrySet() { >>> throw new UnsupportedOperationException("TODO implement me"); >>> // TODO implement me >>> } >>> >>> We should be implementing these methods, shouldn't we? Users might >>> want to iterate the map for example to print the contents or to >>> verify it contains what they put on it...etc. >>> >>> I can't see a JIRA for it, so I'll open one so that this gets >>> implemented for Beta1 unless someone has any objections. >>> >>> On IRC, Mircea said it might very costly, specially for dist, but >>> we still need it regardless, otherwise we're not fully implementing >>> the ConcurrentMap interface and these are commonly used methods. >> for DIST these might be implemented as follows: >> -the node broadcasts a 'GetKeys' command to all others nodes in the >> cluster >> - on remote nodes locks are acquired for all keys (not sure this >> mandatory though) >> - each node responds with the set of keys >> - calling node puts them together and returns them to the user >> for entrySet this is even more costly, as all cluster state shall be >> transfered to the calling node. >> Now, what worries me is the fact that users might overuse these >> methods, without being aware of the performance consequences. >> A possible way to enforce the users to acknowledge that these >> operations are costly, is to disable them by default and allow a >> config element to enable them. > > I don't like the idea of having a public API method disabled by > default. Instead, I think we should warn users when calling these > methods with DIST mode on. I still reckon that these methods should just query the local, in-memory data container. And be documented such that this is what is expected. Like I said the potential for OOMs is huge otherwise - regardless of DIST.

Cheers -- Manik Surtani manik(a)jboss.org Lead, Infinispan Lead, JBoss Cache http://www.infinispan.org http://www.jbosscache.org

Mircea Markus wrote: > Manik Surtani wrote: >> </snip> >> >> I still reckon that these methods should just query the local, >> in-memory data container. And be documented such that this is what >> is expected. Like I said the potential for OOMs is huge otherwise - >> regardless of DIST. > that would 'lay' to the user and break the API as well. We can add > Cache (vs Map) methods for that. Hmmmm, I suppose you mean lie?

As long as we document it correctly, I think we should be fine. We could even show an info message when DIST is in use to further remain them that the view they get only contains the local data. I don't think it breaks the API. It has limitations but I don't a break.

It's a bit like size() with ConcurrentHashMap. It does not give you an accurate number. The same with keySet()/entrySet() and DIST mode, it does not give an accurate view because of the pitfalls mentioned. >> Cheers >> -- >> Manik Surtani >> manik(a)jboss.org >> Lead, Infinispan >> Lead, JBoss Cache >> http://www.infinispan.org >> http://www.jbosscache.org >> >> >> >> >

On 4 Jun 2009, at 10:36, Mircea Markus wrote: > Galder Zamarreno wrote: >> >> >> Mircea Markus wrote: >>> Manik Surtani wrote: >>>> </snip> >>>> >>>> I still reckon that these methods should just query the local, >>>> in-memory data container. And be documented such that this is >>>> what is expected. Like I said the potential for OOMs is huge >>>> otherwise - regardless of DIST. >>> that would 'lay' to the user and break the API as well. We can add >>> Cache (vs Map) methods for that. >> >> Hmmmm, I suppose you mean lie? > yes :) >> As long as we document it correctly, I think we should be fine. We >> could even show an info message when DIST is in use to further >> remain them that the view they get only contains the local data. >> >> I don't think it breaks the API. It has limitations but I don't a >> break. > cache.put(k,v); > asser cache.keySet().contains(k) : "this will fail even within a tx"; This can be a problem with a CHM as well: map.put(k, v) assert map.size() // may not return what you expect

> I generally think that it's better not to give any information than > give incomplete/incorrect one, on the other hand I see the point with > having this local One of the limitations of Cache extending ConcurrentMap. IMO the only real drawback of extending ConcurrentMap. > >> It's a bit like size() with ConcurrentHashMap. It does not give you >> an accurate number. The same with keySet()/entrySet() and DIST mode, >> it does not give an accurate view because of the pitfalls mentioned. >> >>>> Cheers >>>> -- >>>> Manik Surtani >>>> manik(a)jboss.org >>>> Lead, Infinispan >>>> Lead, JBoss Cache >>>> http://www.infinispan.org >>>> http://www.jbosscache.org >>>> >>>> >>>> >>>> >>> >> > -- Manik Surtani manik(a)jboss.org Lead, Infinispan Lead, JBoss Cache http://www.infinispan.org http://www.jbosscache.org

Hi, keySet(), values(), entrySet() do not seem to be implemented at the moment: public Set<K> keySet() { throw new UnsupportedOperationException("TODO implement me"); // TODO implement me } public Collection<V> values() { throw new UnsupportedOperationException("TODO implement me"); // TODO implement me } public Set<Map.Entry<K, V>> entrySet() { throw new UnsupportedOperationException("TODO implement me"); // TODO implement me } We should be implementing these methods, shouldn't we? Users might want to iterate the map for example to print the contents or to verify it contains what they put on it...etc. I can't see a JIRA for it, so I'll open one so that this gets implemented for Beta1 unless someone has any objections. On IRC, Mircea said it might very costly, specially for dist, but we still need it regardless, otherwise we're not fully implementing the ConcurrentMap interface and these are commonly used methods. Regards, -- Galder Zamarreño Sr. Software Maintenance Engineer JBoss, a division of Red Hat _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Manik Surtani wrote: > +1. > Implementing these naively is easy; anything else is much harder. > Here are some basic thoughts. > If we are in LOCAL or REPL mode, this is straightforward *if* we > ignore cache stores. If we take cache stores or DIST into account, > we have the potential for instant OOMs. Even if we use custom > collections that load lazily from a store or across a cluster, the > collections still need to efficiently handle contains(), size(), etc. > Perhaps we should just limit these to reporting what is local, and > in-memory only. Clearly Javadoc'ed, of course, to detail these > limitations - along with a general recommendation that these are > inherited from ConcurrentMap and not expected to be used in any > extensive or meaningful way. +1. I like the idea of only reporting what's available locally + documenting it correctly. > A few other thoughts (and these should be javadoc'ed too): > * These collections should always be immutable > * These methods do not participate in any transactions +1 to both. I volunteer to implement this unless someone else wants to do it?