Performance vs correctness: I vote fore the second

[Vladimir Ozerov]

Evgeniy,

Good catch! I personally had to explain users several time why they loose
data in these cases with default configuration. "AtomicConfiguration.backups
= 0" and "CollectionConfiguration.backups = 0" as defaults is nonsense.

On Thu, Apr 20, 2017 at 3:27 PM, Evgeniy Stanilovskiy <
[hidden email]> wrote:

> Guys, hope i can add one more example here.
> Ones we use IgniteAtomicSequence, after topology changes some assertions
> can be catched due to default AtomicConfiguration
> i.e.
>     public static final int DFLT_BACKUPS = 0;
>     public static final CacheMode DFLT_CACHE_MODE = PARTITIONED;
>
> minimal improvements here would be to set DFLT_BACKUPS = 1; or change into
> REPLICATED mode.
>
> thanks.
>
> Folks,
>>
>> I received a number of complaints from users that our default setting
>> favor
>> performance at the cost of correctness and subtle behavior. Yesterday I
>> faced one such situation on my own.
>>
>> I started REPLICATED cache on several nodes, put some data, executed
>> simple
>> SQL and got wrong result. No errors, no warnings. The problem was caused
>> by
>> default PRIMARY_SYNC mode. WTF, our cache doesn't work out of the box!
>>
>> Another widely known examples are data streamer behavior, "read form
>> backups" + continuous queries.
>>
>> I propose to change our defaults to favor *correctness* over performance,
>> and create good documentation and JavaDocs to explain users how to tune
>> our
>> product. Proposed changes:
>>
>> 1) FULL_SYNC as default;
>> 2) "readFromBackups=false" as default;
>> 3) "IgniteDataStreamer.allowOverwrite=true" as default.
>>
>> Users should not think how to make Ignite work correctly. It should be
>> correct out of the box.
>>
>> Vladimir.
>>
>

[Valentin Kulichenko]

Yeah, that's a very good point. However, the problem here is that we use
single cache for very different structures. For atomics, for example,
partitioned cache makes sense (usually with 1 or more backups though).
While reentrant locks should always be in replicated cache in my view (or
at least by default). Currently it's one or another for both.

-Val



On Thu, Apr 20, 2017 at 2:37 PM, Vladimir Ozerov <[hidden email]>
wrote:

> Evgeniy,
>
> Good catch! I personally had to explain users several time why they loose
> data in these cases with default configuration.
> "AtomicConfiguration.backups
> = 0" and "CollectionConfiguration.backups = 0" as defaults is nonsense.
>
> On Thu, Apr 20, 2017 at 3:27 PM, Evgeniy Stanilovskiy <
> [hidden email]> wrote:
>
> > Guys, hope i can add one more example here.
> > Ones we use IgniteAtomicSequence, after topology changes some assertions
> > can be catched due to default AtomicConfiguration
> > i.e.
> >     public static final int DFLT_BACKUPS = 0;
> >     public static final CacheMode DFLT_CACHE_MODE = PARTITIONED;
> >
> > minimal improvements here would be to set DFLT_BACKUPS = 1; or change
> into
> > REPLICATED mode.
> >
> > thanks.
> >
> > Folks,
> >>
> >> I received a number of complaints from users that our default setting
> >> favor
> >> performance at the cost of correctness and subtle behavior. Yesterday I
> >> faced one such situation on my own.
> >>
> >> I started REPLICATED cache on several nodes, put some data, executed
> >> simple
> >> SQL and got wrong result. No errors, no warnings. The problem was caused
> >> by
> >> default PRIMARY_SYNC mode. WTF, our cache doesn't work out of the box!
> >>
> >> Another widely known examples are data streamer behavior, "read form
> >> backups" + continuous queries.
> >>
> >> I propose to change our defaults to favor *correctness* over
> performance,
> >> and create good documentation and JavaDocs to explain users how to tune
> >> our
> >> product. Proposed changes:
> >>
> >> 1) FULL_SYNC as default;
> >> 2) "readFromBackups=false" as default;
> >> 3) "IgniteDataStreamer.allowOverwrite=true" as default.
> >>
> >> Users should not think how to make Ignite work correctly. It should be
> >> correct out of the box.
> >>
> >> Vladimir.
> >>
> >
>

[Vladimir Ozerov]

Valya,

Why do you think locks should be in REPLICATED cache? It will make their
performance so poor, that users are likely to give using them :-)

On Thu, Apr 20, 2017 at 4:04 PM, Valentin Kulichenko <
[hidden email]> wrote:

> Yeah, that's a very good point. However, the problem here is that we use
> single cache for very different structures. For atomics, for example,
> partitioned cache makes sense (usually with 1 or more backups though).
> While reentrant locks should always be in replicated cache in my view (or
> at least by default). Currently it's one or another for both.
>
> -Val
>
>
>
> On Thu, Apr 20, 2017 at 2:37 PM, Vladimir Ozerov <[hidden email]>
> wrote:
>
> > Evgeniy,
> >
> > Good catch! I personally had to explain users several time why they loose
> > data in these cases with default configuration.
> > "AtomicConfiguration.backups
> > = 0" and "CollectionConfiguration.backups = 0" as defaults is nonsense.
> >
> > On Thu, Apr 20, 2017 at 3:27 PM, Evgeniy Stanilovskiy <
> > [hidden email]> wrote:
> >
> > > Guys, hope i can add one more example here.
> > > Ones we use IgniteAtomicSequence, after topology changes some
> assertions
> > > can be catched due to default AtomicConfiguration
> > > i.e.
> > >     public static final int DFLT_BACKUPS = 0;
> > >     public static final CacheMode DFLT_CACHE_MODE = PARTITIONED;
> > >
> > > minimal improvements here would be to set DFLT_BACKUPS = 1; or change
> > into
> > > REPLICATED mode.
> > >
> > > thanks.
> > >
> > > Folks,
> > >>
> > >> I received a number of complaints from users that our default setting
> > >> favor
> > >> performance at the cost of correctness and subtle behavior. Yesterday
> I
> > >> faced one such situation on my own.
> > >>
> > >> I started REPLICATED cache on several nodes, put some data, executed
> > >> simple
> > >> SQL and got wrong result. No errors, no warnings. The problem was
> caused
> > >> by
> > >> default PRIMARY_SYNC mode. WTF, our cache doesn't work out of the box!
> > >>
> > >> Another widely known examples are data streamer behavior, "read form
> > >> backups" + continuous queries.
> > >>
> > >> I propose to change our defaults to favor *correctness* over
> > performance,
> > >> and create good documentation and JavaDocs to explain users how to
> tune
> > >> our
> > >> product. Proposed changes:
> > >>
> > >> 1) FULL_SYNC as default;
> > >> 2) "readFromBackups=false" as default;
> > >> 3) "IgniteDataStreamer.allowOverwrite=true" as default.
> > >>
> > >> Users should not think how to make Ignite work correctly. It should be
> > >> correct out of the box.
> > >>
> > >> Vladimir.
> > >>
> > >
> >
>

[Valentin Kulichenko]

Well, I agree that's arguable :) But keeping in mind that the cache is also
used for atomics, replicated cache doesn't make sense in any case. Setting
default number of backups to 1 for atomics and collections caches should be
good enough.

-Val

On Thu, Apr 20, 2017 at 3:09 PM, Vladimir Ozerov <[hidden email]>
wrote:

> Valya,
>
> Why do you think locks should be in REPLICATED cache? It will make their
> performance so poor, that users are likely to give using them :-)
>
> On Thu, Apr 20, 2017 at 4:04 PM, Valentin Kulichenko <
> [hidden email]> wrote:
>
> > Yeah, that's a very good point. However, the problem here is that we use
> > single cache for very different structures. For atomics, for example,
> > partitioned cache makes sense (usually with 1 or more backups though).
> > While reentrant locks should always be in replicated cache in my view (or
> > at least by default). Currently it's one or another for both.
> >
> > -Val
> >
> >
> >
> > On Thu, Apr 20, 2017 at 2:37 PM, Vladimir Ozerov <[hidden email]>
> > wrote:
> >
> > > Evgeniy,
> > >
> > > Good catch! I personally had to explain users several time why they
> loose
> > > data in these cases with default configuration.
> > > "AtomicConfiguration.backups
> > > = 0" and "CollectionConfiguration.backups = 0" as defaults is
> nonsense.
> > >
> > > On Thu, Apr 20, 2017 at 3:27 PM, Evgeniy Stanilovskiy <
> > > [hidden email]> wrote:
> > >
> > > > Guys, hope i can add one more example here.
> > > > Ones we use IgniteAtomicSequence, after topology changes some
> > assertions
> > > > can be catched due to default AtomicConfiguration
> > > > i.e.
> > > >     public static final int DFLT_BACKUPS = 0;
> > > >     public static final CacheMode DFLT_CACHE_MODE = PARTITIONED;
> > > >
> > > > minimal improvements here would be to set DFLT_BACKUPS = 1; or change
> > > into
> > > > REPLICATED mode.
> > > >
> > > > thanks.
> > > >
> > > > Folks,
> > > >>
> > > >> I received a number of complaints from users that our default
> setting
> > > >> favor
> > > >> performance at the cost of correctness and subtle behavior.
> Yesterday
> > I
> > > >> faced one such situation on my own.
> > > >>
> > > >> I started REPLICATED cache on several nodes, put some data, executed
> > > >> simple
> > > >> SQL and got wrong result. No errors, no warnings. The problem was
> > caused
> > > >> by
> > > >> default PRIMARY_SYNC mode. WTF, our cache doesn't work out of the
> box!
> > > >>
> > > >> Another widely known examples are data streamer behavior, "read form
> > > >> backups" + continuous queries.
> > > >>
> > > >> I propose to change our defaults to favor *correctness* over
> > > performance,
> > > >> and create good documentation and JavaDocs to explain users how to
> > tune
> > > >> our
> > > >> product. Proposed changes:
> > > >>
> > > >> 1) FULL_SYNC as default;
> > > >> 2) "readFromBackups=false" as default;
> > > >> 3) "IgniteDataStreamer.allowOverwrite=true" as default.
> > > >>
> > > >> Users should not think how to make Ignite work correctly. It should
> be
> > > >> correct out of the box.
> > > >>
> > > >> Vladimir.
> > > >>
> > > >
> > >
> >
>