IGNITE-1355 Potential NPE in CacheAffinityProxy

[Artem Shutak]

Igniters,

I'm working on https://issues.apache.org/jira/browse/IGNITE-1355.

I want to hear a community opinion what should do Ignite in case when user
uses Affinity for nonexistent cache? Current implementation returns
AffinityProxy on a call of Ignite.affinity("nonexistent_cache") and
AffinityProxy throws NPE on a call of any method if cache has not been
created yet.

I see next possible decisions:
- Ignite.affinity("nonexistent_cache") can return 'null' instead of
AffinityProxy like Ignite does it for Ignite.cache("nonexistent_cache").
But it breaks backward compatibility.
- AffinityProxy methods can return a special value like 0 for
'partitions()' method  and empty array for 'primaryPartitions(ClusterNode)'
method.
- AffinityProxy methods can throw Exception that cache does not exist.

I vote for the second one.

Thoughts?

Thanks,
-- Artem --

[Artem Shutak]

I need to add that in the next scenario AffinityImpl will be used (not
AffinityProxy) and the code throws NPE too.

ignite.getOrCreateCache(new CacheConfiguration("myCache"));

Affinity<Object> affinity = ignite.affinity("myCache");

affinity.mapPartitionToNode(0); // All ok.

ignite2.cache("myCache").destroy();

affinity.mapPartitionToNode(0); // NPE.

This case looks like an exception for me, so I need to change my opinion
and I vote for throwing a special exception if cache does not exists.

-- Artem --

On Thu, Nov 12, 2015 at 4:01 PM, Artem Shutak <[hidden email]> wrote:

> Igniters,
>
> I'm working on https://issues.apache.org/jira/browse/IGNITE-1355.
>
> I want to hear a community opinion what should do Ignite in case when user
> uses Affinity for nonexistent cache? Current implementation returns
> AffinityProxy on a call of Ignite.affinity("nonexistent_cache") and
> AffinityProxy throws NPE on a call of any method if cache has not been
> created yet.
>
> I see next possible decisions:
> - Ignite.affinity("nonexistent_cache") can return 'null' instead of
> AffinityProxy like Ignite does it for Ignite.cache("nonexistent_cache").
> But it breaks backward compatibility.
> - AffinityProxy methods can return a special value like 0 for
> 'partitions()' method  and empty array for 'primaryPartitions(ClusterNode)'
> method.
> - AffinityProxy methods can throw Exception that cache does not exist.
>
> I vote for the second one.
>
> Thoughts?
>
> Thanks,
> -- Artem --
>

[yzhdanov]

Any cache can be destroyed after proxy is created, so any cache may become
unexistent at any moment. So, I would still allow proxy creation for
unexistent caches, but throw an exception if cache does not exist at the
moment of proxy method call.

--Yakov

2015-11-12 16:01 GMT+03:00 Artem Shutak <[hidden email]>:

> Igniters,
>
> I'm working on https://issues.apache.org/jira/browse/IGNITE-1355.
>
> I want to hear a community opinion what should do Ignite in case when user
> uses Affinity for nonexistent cache? Current implementation returns
> AffinityProxy on a call of Ignite.affinity("nonexistent_cache") and
> AffinityProxy throws NPE on a call of any method if cache has not been
> created yet.
>
> I see next possible decisions:
> - Ignite.affinity("nonexistent_cache") can return 'null' instead of
> AffinityProxy like Ignite does it for Ignite.cache("nonexistent_cache").
> But it breaks backward compatibility.
> - AffinityProxy methods can return a special value like 0 for
> 'partitions()' method  and empty array for 'primaryPartitions(ClusterNode)'
> method.
> - AffinityProxy methods can throw Exception that cache does not exist.
>
> I vote for the second one.
>
> Thoughts?
>
> Thanks,
> -- Artem --
>

[Artem Shutak]

Igniters,

I've fixed the issue according to the last discussed approach [1].

I've faced with the following API issues:
- IgniteCluster and Affinity interfaces have the same methods:
mapKeysToNodes and mapKeyToNode, which use the same implementation.
- The both methods in both interfaces say that they return special value in
case when cache doesn't exist (empty map for 'mapKeysToNodes' and null for
'mapKeyToNode').

I want do the following (already did in my PR [1]):
1. Deprecate the methods in IgniteCluster interface.
2. Change javadoc of the methods on Affinity interface. Javadoc will say
that an exception will be thrown if cache does not exist.

The problem is the point 2 breaks backward compatibility. But as I see from
the methods implementation, they can throw IgniteException in some cases
now, so a user should already have processing of the exception. So, I think
we can do these changes.

Thoughts?

[1] https://github.com/apache/ignite/pull/263

Thanks,
-- Artem --

On Thu, Nov 12, 2015 at 4:10 PM, Yakov Zhdanov <[hidden email]> wrote:

> Any cache can be destroyed after proxy is created, so any cache may become
> unexistent at any moment. So, I would still allow proxy creation for
> unexistent caches, but throw an exception if cache does not exist at the
> moment of proxy method call.
>
> --Yakov
>
> 2015-11-12 16:01 GMT+03:00 Artem Shutak <[hidden email]>:
>
> > Igniters,
> >
> > I'm working on https://issues.apache.org/jira/browse/IGNITE-1355.
> >
> > I want to hear a community opinion what should do Ignite in case when
> user
> > uses Affinity for nonexistent cache? Current implementation returns
> > AffinityProxy on a call of Ignite.affinity("nonexistent_cache") and
> > AffinityProxy throws NPE on a call of any method if cache has not been
> > created yet.
> >
> > I see next possible decisions:
> > - Ignite.affinity("nonexistent_cache") can return 'null' instead of
> > AffinityProxy like Ignite does it for Ignite.cache("nonexistent_cache").
> > But it breaks backward compatibility.
> > - AffinityProxy methods can return a special value like 0 for
> > 'partitions()' method  and empty array for
> 'primaryPartitions(ClusterNode)'
> > method.
> > - AffinityProxy methods can throw Exception that cache does not exist.
> >
> > I vote for the second one.
> >
> > Thoughts?
> >
> > Thanks,
> > -- Artem --
> >
>

[yzhdanov]

I agree with proposed changes.

--Yakov

2015-11-25 16:35 GMT+03:00 Artem Shutak <[hidden email]>:

> Igniters,
>
> I've fixed the issue according to the last discussed approach [1].
>
> I've faced with the following API issues:
> - IgniteCluster and Affinity interfaces have the same methods:
> mapKeysToNodes and mapKeyToNode, which use the same implementation.
> - The both methods in both interfaces say that they return special value in
> case when cache doesn't exist (empty map for 'mapKeysToNodes' and null for
> 'mapKeyToNode').
>
> I want do the following (already did in my PR [1]):
> 1. Deprecate the methods in IgniteCluster interface.
> 2. Change javadoc of the methods on Affinity interface. Javadoc will say
> that an exception will be thrown if cache does not exist.
>
> The problem is the point 2 breaks backward compatibility. But as I see from
> the methods implementation, they can throw IgniteException in some cases
> now, so a user should already have processing of the exception. So, I think
> we can do these changes.
>
> Thoughts?
>
> [1] https://github.com/apache/ignite/pull/263
>
> Thanks,
> -- Artem --
>
> On Thu, Nov 12, 2015 at 4:10 PM, Yakov Zhdanov <[hidden email]>
> wrote:
>
> > Any cache can be destroyed after proxy is created, so any cache may
> become
> > unexistent at any moment. So, I would still allow proxy creation for
> > unexistent caches, but throw an exception if cache does not exist at the
> > moment of proxy method call.
> >
> > --Yakov
> >
> > 2015-11-12 16:01 GMT+03:00 Artem Shutak <[hidden email]>:
> >
> > > Igniters,
> > >
> > > I'm working on https://issues.apache.org/jira/browse/IGNITE-1355.
> > >
> > > I want to hear a community opinion what should do Ignite in case when
> > user
> > > uses Affinity for nonexistent cache? Current implementation returns
> > > AffinityProxy on a call of Ignite.affinity("nonexistent_cache") and
> > > AffinityProxy throws NPE on a call of any method if cache has not been
> > > created yet.
> > >
> > > I see next possible decisions:
> > > - Ignite.affinity("nonexistent_cache") can return 'null' instead of
> > > AffinityProxy like Ignite does it for
> Ignite.cache("nonexistent_cache").
> > > But it breaks backward compatibility.
> > > - AffinityProxy methods can return a special value like 0 for
> > > 'partitions()' method  and empty array for
> > 'primaryPartitions(ClusterNode)'
> > > method.
> > > - AffinityProxy methods can throw Exception that cache does not exist.
> > >
> > > I vote for the second one.
> > >
> > > Thoughts?
> > >
> > > Thanks,
> > > -- Artem --
> > >
> >
>