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? |
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 <[hidden email]> 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? |
In reply to this post by Vladimir Ozerov
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 <[hidden email]> 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? > |
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 <[hidden email]> 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 <[hidden email]> > 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? > > > |
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 < [hidden email]> 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 <[hidden email]> > 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 <[hidden email]> > > 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? > > > > > > |
Free forum by Nabble | Edit this page |