Deprecate CacheConfiguration.setIndexedTypes

[Vladimir Ozerov <vo...@gridgain.com>]

Igniters,

I'd like to propose to deprecate infamous CacheConfiguration.setIndexedTypes
method. It has subtle semantics and clashes with
CacheConfiguration.setQueryEntities method. Several examples.

Example 1
setIndexedTypes(Key1.class, Value1.class);
setIndexedTypes(Key2.class, Value2.class);
=> exeption

Example 2
setIndexedTypes(Key1.class, Value1.class);
setQueryEntitities(new QueryEntity(...));
=> all ok, two query entities

Proposal:
1) Deprecate CacheConfiguration.setIndexedTypes
2) Add new constructor QueryEntity(Class keyCls, Class valCls)
3) Remove all non-obvious semanrics from
CacheConfiguration.setQueryEntities method
and make it plain setter.

Thoughts?

Re: Deprecate CacheConfiguration.setIndexedTypes

[Denis Magda <dm...@apache.org>]

Vladimir,

I’m fully for your idea. In general, as soon as our DDL becomes mature enough we might want to simplify or get rid off current APIs that define SQL schema.

—
Denis

> On Apr 19, 2017, at 1:22 PM, Vladimir Ozerov <vo...@gridgain.com> wrote:
> 
> Igniters,
> 
> I'd like to propose to deprecate infamous CacheConfiguration.setIndexedTypes
> method. It has subtle semantics and clashes with
> CacheConfiguration.setQueryEntities method. Several examples.
> 
> Example 1
> setIndexedTypes(Key1.class, Value1.class);
> setIndexedTypes(Key2.class, Value2.class);
> => exeption
> 
> Example 2
> setIndexedTypes(Key1.class, Value1.class);
> setQueryEntitities(new QueryEntity(...));
> => all ok, two query entities
> 
> Proposal:
> 1) Deprecate CacheConfiguration.setIndexedTypes
> 2) Add new constructor QueryEntity(Class keyCls, Class valCls)
> 3) Remove all non-obvious semanrics from
> CacheConfiguration.setQueryEntities method
> and make it plain setter.
> 
> Thoughts?

Re: Deprecate CacheConfiguration.setIndexedTypes

[Vladimir Ozerov <vo...@gridgain.com>]

Valya,

Normally setters do not throw exceptions. It is perfectly fine to override
something. This is how 99% of our configuration works. In this case
exception is thrown because indexed types are converted to query entities
inside the setter, so the next call to this setter cannot understand what
to do.

As per QueryEntity, I do not think we should throw exceptions. This
constructor is merely a shortcut which extracts data from classes. It
appears perfectly valid to me to, for example, add another index or column
after that. Especially provided that we already have dynamic index
create/drop, and will have CREATE/ALTER/DROP table later.

Vladimir.

On Thu, Apr 20, 2017 at 10:17 AM, Valentin Kulichenko <
valentin.kulichenko@gmail.com> wrote:

> Exception is thrown for safety if setIndexedTypes is called more than once.
> Otherwise second call will override the first one and Key1-Value1 will be
> ignored. The correct code should look like this:
>
> setIndexedTypes(Key1.class, Value1.class, Key2.class, Value2.class);
>
> Generally, I agree with this change, but with one minor concern. What will
> happen if I use new QueryEntity constructor (QueryEntity(Class keyCls,
> Class valCls)), and then use setters to modify this entity? I think we
> should throw an exception in this case.
>
> -Val
>
> On Thu, Apr 20, 2017 at 2:02 AM, Dmitriy Setrakyan <ds...@apache.org>
> wrote:
>
> > Vladimir, I am not sure I understand the issue here. Why would the
> Example
> > 1 result in an exception?
> >
> > On Wed, Apr 19, 2017 at 1:22 PM, Vladimir Ozerov <vo...@gridgain.com>
> > wrote:
> >
> > > Igniters,
> > >
> > > I'd like to propose to deprecate infamous
> CacheConfiguration.setIndexedT
> > > ypes
> > > method. It has subtle semantics and clashes with
> > > CacheConfiguration.setQueryEntities method. Several examples.
> > >
> > > Example 1
> > > setIndexedTypes(Key1.class, Value1.class);
> > > setIndexedTypes(Key2.class, Value2.class);
> > > => exeption
> > >
> > > Example 2
> > > setIndexedTypes(Key1.class, Value1.class);
> > > setQueryEntitities(new QueryEntity(...));
> > > => all ok, two query entities
> > >
> > > Proposal:
> > > 1) Deprecate CacheConfiguration.setIndexedTypes
> > > 2) Add new constructor QueryEntity(Class keyCls, Class valCls)
> > > 3) Remove all non-obvious semanrics from
> > > CacheConfiguration.setQueryEntities method
> > > and make it plain setter.
> > >
> > > Thoughts?
> > >
> >
>

Re: Deprecate CacheConfiguration.setIndexedTypes

[Valentin Kulichenko <va...@gmail.com>]

Exception is thrown for safety if setIndexedTypes is called more than once.
Otherwise second call will override the first one and Key1-Value1 will be
ignored. The correct code should look like this:

setIndexedTypes(Key1.class, Value1.class, Key2.class, Value2.class);

Generally, I agree with this change, but with one minor concern. What will
happen if I use new QueryEntity constructor (QueryEntity(Class keyCls,
Class valCls)), and then use setters to modify this entity? I think we
should throw an exception in this case.

-Val

On Thu, Apr 20, 2017 at 2:02 AM, Dmitriy Setrakyan <ds...@apache.org>
wrote:

> Vladimir, I am not sure I understand the issue here. Why would the Example
> 1 result in an exception?
>
> On Wed, Apr 19, 2017 at 1:22 PM, Vladimir Ozerov <vo...@gridgain.com>
> wrote:
>
> > Igniters,
> >
> > I'd like to propose to deprecate infamous CacheConfiguration.setIndexedT
> > ypes
> > method. It has subtle semantics and clashes with
> > CacheConfiguration.setQueryEntities method. Several examples.
> >
> > Example 1
> > setIndexedTypes(Key1.class, Value1.class);
> > setIndexedTypes(Key2.class, Value2.class);
> > => exeption
> >
> > Example 2
> > setIndexedTypes(Key1.class, Value1.class);
> > setQueryEntitities(new QueryEntity(...));
> > => all ok, two query entities
> >
> > Proposal:
> > 1) Deprecate CacheConfiguration.setIndexedTypes
> > 2) Add new constructor QueryEntity(Class keyCls, Class valCls)
> > 3) Remove all non-obvious semanrics from
> > CacheConfiguration.setQueryEntities method
> > and make it plain setter.
> >
> > Thoughts?
> >
>

Re: Deprecate CacheConfiguration.setIndexedTypes

[Dmitriy Setrakyan <ds...@apache.org>]

Vladimir, I am not sure I understand the issue here. Why would the Example
1 result in an exception?

On Wed, Apr 19, 2017 at 1:22 PM, Vladimir Ozerov <vo...@gridgain.com>
wrote:

> Igniters,
>
> I'd like to propose to deprecate infamous CacheConfiguration.setIndexedT
> ypes
> method. It has subtle semantics and clashes with
> CacheConfiguration.setQueryEntities method. Several examples.
>
> Example 1
> setIndexedTypes(Key1.class, Value1.class);
> setIndexedTypes(Key2.class, Value2.class);
> => exeption
>
> Example 2
> setIndexedTypes(Key1.class, Value1.class);
> setQueryEntitities(new QueryEntity(...));
> => all ok, two query entities
>
> Proposal:
> 1) Deprecate CacheConfiguration.setIndexedTypes
> 2) Add new constructor QueryEntity(Class keyCls, Class valCls)
> 3) Remove all non-obvious semanrics from
> CacheConfiguration.setQueryEntities method
> and make it plain setter.
>
> Thoughts?
>