[
https://issues.apache.org/jira/browse/IGNITE-640?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16479283#comment-16479283
]
Anton Vinogradov commented on IGNITE-640:
-----------------------------------------
Hi, [~aakhmedov]
I made a brief review, here's some comments:
0) We have an issue IGNITE-7823 which affects your changes.
Added Issue as related.
BTW, current assignee have some issues with implementation, do you intereasted
to fix this too?
1) I see some architecture duplication.
{{Map}} and {{Set}} is the same thing, I think.
So, it will be a good idea to have some common implementation and {{set}} +
{{map}} based on it.
2) Also, I see you made changes similar to {{Queue}} code.
It will be nice to fix this duplication too.
3) I see some codestyle issues.
Main point is that each semantic unit should be separated by new line.
Example:
{code:java}
/** {@inheritDoc} */
@Override public List<V> get(K key, int min, int max) {
try {
List<V> list = cache.get(itemKey(key));
//newline
if (list == null)
return Collections.emptyList();
//newline
return IntStream.rangeClosed(min, max)
.mapToObj(list::get)
.collect(toList());
}
catch (IgniteCheckedException e) {
throw U.convertException(e);
}
}
{code}
4) Currently java8 features allowed only at tests.
That does not mean you should replace all code by java7 code.
My proposal is to discuss this one more time.
previous discussion:
[http://apache-ignite-developers.2346864.n4.nabble.com/Use-of-lambdas-and-other-Java-8-features-td27671.html#a27674]
let's continue it
I agree that
{code:java}
return IntStream.rangeClosed(min, max)
.mapToObj(list::get)
.collect(toList());
{code}
looks better than java7 code, but not sure it has same speed.
5) I see duplication at tests, for example
{{GridCacheLocalAtomicCollocatedMultimapApiSelfTest}} pretty similar to
{{GridCacheLocalAtomicNonCollocatedMultimapApiSelfTest}}.
6) I see duplication at GridCacheMultimapApiSelfAbstractTest, looks like this
test can be shorten.
Here is ans example how to make test smaller:
{{org.apache.ignite.internal.processors.cache.CrossCacheTxRandomOperationsTest#cacheOperation}}
7) I see some lines commented
{code:java}
// multimap1.close();
// multimap2.close();
// multimap3.close();
{code}
final patch should not contain commented lines.
8) I created Upsource review to see you code at IDEA, but Upsource works bad
with merges from master (which you have at branch), so, following request's
goal it to make my review easy ;)
Could you please merge current master to your branch and then squash your
branch with force commit (to gain only one commit with your changes). This will
allow Upsource show same changes as at PR.
You should not make any more merges or squashes after that, this will break
Upsource again. Each subsequent fix should be as a separate commit. Thanks.
> Implement IgniteMultimap data structures
> ----------------------------------------
>
> Key: IGNITE-640
> URL: https://issues.apache.org/jira/browse/IGNITE-640
> Project: Ignite
> Issue Type: Sub-task
> Components: data structures
> Reporter: Dmitriy Setrakyan
> Assignee: Amir Akhmedov
> Priority: Major
> Fix For: 2.6
>
>
> We need to add {{IgniteMultimap}} data structure in addition to other data
> structures provided by Ignite. {{IgniteMultiMap}} should have similar API to
> {{java.util.Map}} class in JDK, but support the semantics of multiple values
> per key, similar to [Guava
> Multimap|http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/collect/Multimap.html].
>
> However, unlike in Guava, our multi-map should work with Lists, not
> Collections. Lists should make it possible to support the following methods:
> {code}
> // Gets value at a certain index for a key.
> V get(K, index);
> // Gets all values for a collection of keys at a certain index.
> Map<K, V> getAll(Collection<K>, index);
> // Gets values for specified indexes for a key.
> List<V> get(K, Iterable<Integer> indexes);
> // Gets all values for a collection of keys at specified indexes.
> Map<K, Collection<V>> getAll(Collection<K>, Iterable<Integer> indexes);
> // Gets values for specified range of indexes, between min and max.
> List<V> get(K, int min, int max);
> // Gets all values for a collection of keys for a specified index range,
> between min and max.
> Map<K, Collection<V>> getAll(Collection<K>, int min, int max);
> // Gets all values for a specific key.
> List<V> get(K);
> // Gets all values for a collection of keys.
> Map<K, List<V>> getAll(Collection<K>);
> // Iterate through all elements with a certain index.
> Iterator<Map.Entry<K, V>> iterate(int idx);
> // Do we need this?
> Collection<IgniteTuple<Integer V>> get(K, IgniteBiPredicate<Integer, V>)
> {code}
> Multimap should also support colocated and non-colocated modes, similar to
> [IgniteQueue|https://github.com/apache/incubator-ignite/blob/master/modules/core/src/main/java/org/apache/ignite/IgniteQueue.java]
> and its implementation,
> [GridAtomicCacheQueueImpl|https://github.com/apache/incubator-ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/processors/datastructures/GridAtomicCacheQueueImpl.java].
> h2. Design Details
> The most natural way to implement such map, would be to store every value
> under a separate key in an Ignite cache. For example, let's say that we have
> a key {{K}} with multiple values: {{V0, V1, V2, ...}}. Then the cache should
> end up with the following values {{K0, V0}}, {{K1, V1}}, {{K2, V2}}, etc.
> This means that we need to wrap user key into our own, internal key, which
> will also have {{index}} field.
> Also note that we need to collocate all the values for the same key on the
> same node, which means that we need to define user key K as the affinity key,
> like so:
> {code}
> class MultiKey<K> {
> @CacheAffinityMapped
> private K key;
> int index;
> }
> {code}
> Look ups of values at specific indexes becomes very simple. Just attach a
> specific index to a key and do a cache lookup. Look ups for all values for a
> key should work as following:
> {code}
> MultiKey key;
> V v = null;
> int index = 0;
> List<V> res = new LinkedList<>();
> do {
> v = cache.get(MultiKey(K, index));
> if (v != null)
> res.add(v);
> index++;
> }
> while (v != null);
> return res;
> {code}
> We could also use batching for performance reason. In this case the batch
> size should be configurable.
> {code}
> int index = 0;
> List<V> res = new LinkedList<>();
> while (true) {
> List<Key> batch = new ArrayList<>(batchSize);
> // Populate batch.
> for (; index < batchSize; index++)
> batch.add(new MultiKey(K, index % batchSize);
> Map<Key, V> batchRes = cache.getAll(batch);
> // Potentially need to properly sort values, based on the key order,
> // if the returning map does not do it automatically.
> res.addAll(batchRes.values());
> if (res.size() < batch.size())
> break;
> }
> return res;
> {code}
> h2. Evictions
> Evictions in the {{IgniteMultiMap}} should have 2 levels: maximum number of
> keys, and maximum number of values for a key. The maximum number of keys
> should be controlled by Ignite standard eviction policy. The maximum number
> of values for a key should be controlled by the implementation of the
> multi-map. Either eviction parameter should be configurable.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
