[jira] [Commented] (IGNITE-7883) Cluster can have inconsistent affinity configuration

["Andrey Gura (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/IGNITE-7883?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16802903#comment-16802903 ] 

Andrey Gura commented on IGNITE-7883:
-------------------------------------

[~a-polyakov] I've took a look ti the change and have some comments:

1. {{GridCacheUtils#validateAffinityKeyConfiguration(CacheKeyConfiguration[])}}

I think better name is {{validateKeyConfigiration}}.

Please fix message of {{IgniteCheckedException}}. At the moment it is too verbose and unclear. I think it should be something like "Cache key configuration contains conflicting definitions: [cacheGroup=<>, cache=<>, typeName=<>, affKeyFieldName1=<>, affKeyFieldName2=<>]".

Javadoc should have more accurate formulation. E..g it's unclear words "items" and "full". Proper comment will be "all fields are initialized and not empty". Please rewrite javadoc.

2. {{GridCacheUtils#validateAffinityKeyConfiguration(String, UUID, CacheKeyConfiguration[], CacheKeyConfiguration[])}}

I think better name is {{validateKeyConfigiration}}. 

{{oldCacheKeyCfgs}} and {{newCacheKeyCfgs}} parameters should be renamed to {{rmtCacheKeyCfgs}} and {{locCacheKeyCfgs}} respectively.

Please fix message of {{IgniteCheckedException}}. At the moment it is too verbose and unclear. For example see message that generates {{GridCacheUtils#checkAttributeMismatch}} method. Also note that it could be exception or just log message at warning level (it depends on {{IGNITE_SKIP_CONFIGURATION_CONSISTENCY_CHECK}} system property). Also see p. 1 above.

Format method's code properly.

Fix javadoc in order to properly describe method parameters after renaming.

3. {{CacheKeyConfiguration#equals}}

Use {{Objects.equals}} instead of too long expressions at the method end.

4. {{CacheAffinityKeyConfigurationMismatchTest}}

Please, rewrite javadoc o the class. 

Reformat code. Comma should always be on the same line where previous method parameter placed.

Rewrite test code. Framework has  set of methods for getting ignite and cache configuration. So methods like {getIgnite}} are redundant. See {{GridAbstractTest#getConfiguration()}} and other ignite tests for example.

Add additional tests for teh following cases:
- testKeyConfigurationLengthMismatch  - you test only case when we have key configuration on one node and don't have on another. What about different key configurations on nodes?
- testKeyConfigurationDuplicateTypeName - you test only case for one node. What about conflicting defintions from different nodes?

It makes sense to add test cases for configurations that were constructed due to using {{@AffinityKeyMapped}} annotation.



> Cluster can have inconsistent affinity configuration 
> -----------------------------------------------------
>
>                 Key: IGNITE-7883
>                 URL: https://issues.apache.org/jira/browse/IGNITE-7883
>             Project: Ignite
>          Issue Type: Bug
>          Components: cache
>    Affects Versions: 2.3
>            Reporter: Mikhail Cherkasov
>            Assignee: Alexand Polyakov
>            Priority: Major
>             Fix For: 2.8
>
>          Time Spent: 0.5h
>  Remaining Estimate: 0h
>
> A cluster can have inconsistent affinity configuration if you created two nodes, one with affinity key configuration and other without it(in IgniteCfg or CacheCfg),  both nodes will work fine with no exceptions, but in the same time they will apply different affinity rules to keys:
>  
> {code:java}
> package affinity;
> import org.apache.ignite.Ignite;
> import org.apache.ignite.Ignition;
> import org.apache.ignite.cache.CacheAtomicityMode;
> import org.apache.ignite.cache.CacheKeyConfiguration;
> import org.apache.ignite.cache.CacheMode;
> import org.apache.ignite.cache.affinity.Affinity;
> import org.apache.ignite.configuration.CacheConfiguration;
> import org.apache.ignite.configuration.IgniteConfiguration;
> import org.apache.ignite.spi.discovery.tcp.TcpDiscoverySpi;
> import org.apache.ignite.spi.discovery.tcp.ipfinder.vm.TcpDiscoveryVmIpFinder;
> import java.util.Arrays;
> public class Test {
>     private static int id = 0;
>     public static void main(String[] args) {
>         Ignite ignite = Ignition.start(getConfiguration(true, false));
>         Ignite ignite2 = Ignition.start(getConfiguration(false, false));
>         Affinity<Object> affinity = ignite.affinity("TEST");
>         Affinity<Object> affinity2 = ignite2.affinity("TEST");
>         for (int i = 0; i < 1_000_000; i++) {
>             AKey key = new AKey(i);
>             if(affinity.partition(key) != affinity2.partition(key))
>                 System.out.println("FAILED for: " + key);
>         }
>         System.out.println("DONE");
>     }
>     private static IgniteConfiguration getConfiguration(boolean withAffinityCfg, boolean client) {
>         IgniteConfiguration cfg = new IgniteConfiguration();
>         TcpDiscoveryVmIpFinder finder = new TcpDiscoveryVmIpFinder(true);
>         finder.setAddresses(Arrays.asList("localhost:47500..47600"));
>         cfg.setClientMode(client);
>         cfg.setIgniteInstanceName("test" + id++);
>         CacheConfiguration cacheCfg = new CacheConfiguration("TEST");
>         cacheCfg.setAtomicityMode(CacheAtomicityMode.TRANSACTIONAL);
>         cacheCfg.setCacheMode(CacheMode.PARTITIONED);
>         if(withAffinityCfg) {
>             cacheCfg.setKeyConfiguration(new CacheKeyConfiguration("affinity.AKey", "a"));
>         }
>         cfg.setCacheConfiguration(cacheCfg);
>         cfg.setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(finder));
>         return cfg;
>     }
> }
> class AKey {
>     int a;
>     public AKey(int a) {
>         this.a = a;
>     }
>     @Override public String toString() {
>         return "AKey{" +
>                 "a=" + a +
>                 '}';
>     }
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)