IGNITE-4535 : Add option to store deserialized values on-heap

[Ilya Lantukh]

Hi Igniters,

Since review of IGNITE-4535
<https://issues.apache.org/jira/browse/IGNITE-4535> implementation caused
some misunderstandings, I'd like to open a discussion here and see if
everyone agrees with the chosen approach or can suggest a better one.

We are going to re-use existing EvictionPolicy mechanics to decide when
entry is going to be evicted from on-heap cache. If evictionPolicy == null,
we assume that there is no on-heap cache. One of suggested alternatives was
to have a separate boolean parameter that will enable on-heap cache.

Another questionable decision was to remove tests for memory mode
variations. For example, we had GridCacheContinuousQueryAtomicSelfTest,
GridCacheContinuousQueryAtomicOffheapTieredSelfTest and
GridCacheContinuousQueryAtomicOffheapValuesSelfTest that were testing the
same functionallity for ONHEAP_TIERED, OFFHEAP_TIERED and OFFHEAP_VALUES
modes, respectively. Since those memory modes were removed, only
GridCacheContinuousQueryAtomicSelfTest was left and it now runs in off-heap
mode without on-heap cache. One of suggestions was to add a new subclass to
this test (and all other tests) that will run the same test case with
on-heap cache enabled. In my opinion, functionallity that is specific for
on-heap cache should be tested in completely separate tests (which we
already have), and there is no need to run generic tests with every
possible configuration.

What do you think?

--
Best regards,
Ilya

[dsetrakyan]

Ilya, I looked at the Semyon's comments in the ticket, and I think I agree
with him on all counts.

1. Setting an eviction policy should not be a mechanism to enable the
on-heap cache. We already have eviction policies off-heap as well, and they
don't enable anything. On top of that, the eviction policy should not be a
requirement for the on-heap cache. User should still be able to enable the
on-heap cache, even if it grows indefinitely without evictions. We should
have a more intuitive flag here.

2. As far as the tests go, they should examine all the tests and adapt them
to the new behavior. I think we should have a replica of all off-heap tests
to test the scenario with on-heap caches.

D.

On Tue, Apr 4, 2017 at 8:12 AM, Ilya Lantukh <[hidden email]> wrote:

> Hi Igniters,
>
> Since review of IGNITE-4535
> <https://issues.apache.org/jira/browse/IGNITE-4535> implementation caused
> some misunderstandings, I'd like to open a discussion here and see if
> everyone agrees with the chosen approach or can suggest a better one.
>
> We are going to re-use existing EvictionPolicy mechanics to decide when
> entry is going to be evicted from on-heap cache. If evictionPolicy == null,
> we assume that there is no on-heap cache. One of suggested alternatives was
> to have a separate boolean parameter that will enable on-heap cache.
>
> Another questionable decision was to remove tests for memory mode
> variations. For example, we had GridCacheContinuousQueryAtomicSelfTest,
> GridCacheContinuousQueryAtomicOffheapTieredSelfTest and
> GridCacheContinuousQueryAtomicOffheapValuesSelfTest that were testing the
> same functionallity for ONHEAP_TIERED, OFFHEAP_TIERED and OFFHEAP_VALUES
> modes, respectively. Since those memory modes were removed, only
> GridCacheContinuousQueryAtomicSelfTest was left and it now runs in
> off-heap
> mode without on-heap cache. One of suggestions was to add a new subclass to
> this test (and all other tests) that will run the same test case with
> on-heap cache enabled. In my opinion, functionallity that is specific for
> on-heap cache should be tested in completely separate tests (which we
> already have), and there is no need to run generic tests with every
> possible configuration.
>
> What do you think?
>
> --
> Best regards,
> Ilya
>

[Ilya Lantukh]

Dmitry,

1. Setting an eviction policy should not be a mechanism to enable the
> on-heap cache. We already have eviction policies off-heap as well, and they
> don't enable anything. On top of that, the eviction policy should not be a
> requirement for the on-heap cache. User should still be able to enable the
> on-heap cache, even if it grows indefinitely without evictions. We should
> have a more intuitive flag here.


It doesn't make any sence to me to enable on-heap cache without evictions:
it will result in having all data both in on-heap and off-heap memory. If
we want to support such use case, we should implement a separate on-heap
only mode with page memory disabled.


On Tue, Apr 4, 2017 at 7:15 PM, Dmitriy Setrakyan <[hidden email]>
wrote:

> Ilya, I looked at the Semyon's comments in the ticket, and I think I agree
> with him on all counts.
>
> 1. Setting an eviction policy should not be a mechanism to enable the
> on-heap cache. We already have eviction policies off-heap as well, and they
> don't enable anything. On top of that, the eviction policy should not be a
> requirement for the on-heap cache. User should still be able to enable the
> on-heap cache, even if it grows indefinitely without evictions. We should
> have a more intuitive flag here.
>
> 2. As far as the tests go, they should examine all the tests and adapt them
> to the new behavior. I think we should have a replica of all off-heap tests
> to test the scenario with on-heap caches.
>
> D.
>
> On Tue, Apr 4, 2017 at 8:12 AM, Ilya Lantukh <[hidden email]>
> wrote:
>
> > Hi Igniters,
> >
> > Since review of IGNITE-4535
> > <https://issues.apache.org/jira/browse/IGNITE-4535> implementation
> caused
> > some misunderstandings, I'd like to open a discussion here and see if
> > everyone agrees with the chosen approach or can suggest a better one.
> >
> > We are going to re-use existing EvictionPolicy mechanics to decide when
> > entry is going to be evicted from on-heap cache. If evictionPolicy ==
> null,
> > we assume that there is no on-heap cache. One of suggested alternatives
> was
> > to have a separate boolean parameter that will enable on-heap cache.
> >
> > Another questionable decision was to remove tests for memory mode
> > variations. For example, we had GridCacheContinuousQueryAtomicSelfTest,
> > GridCacheContinuousQueryAtomicOffheapTieredSelfTest and
> > GridCacheContinuousQueryAtomicOffheapValuesSelfTest that were testing
> the
> > same functionallity for ONHEAP_TIERED, OFFHEAP_TIERED and OFFHEAP_VALUES
> > modes, respectively. Since those memory modes were removed, only
> > GridCacheContinuousQueryAtomicSelfTest was left and it now runs in
> > off-heap
> > mode without on-heap cache. One of suggestions was to add a new subclass
> to
> > this test (and all other tests) that will run the same test case with
> > on-heap cache enabled. In my opinion, functionallity that is specific for
> > on-heap cache should be tested in completely separate tests (which we
> > already have), and there is no need to run generic tests with every
> > possible configuration.
> >
> > What do you think?
> >
> > --
> > Best regards,
> > Ilya
> >
>

--
Best regards,
Ilya

[Sergi]

I think having pure on-heap cache is good idea.

Sergi

2017-04-05 12:52 GMT+03:00 Ilya Lantukh <[hidden email]>:

> Dmitry,
>
> 1. Setting an eviction policy should not be a mechanism to enable the
> > on-heap cache. We already have eviction policies off-heap as well, and
> they
> > don't enable anything. On top of that, the eviction policy should not be
> a
> > requirement for the on-heap cache. User should still be able to enable
> the
> > on-heap cache, even if it grows indefinitely without evictions. We should
> > have a more intuitive flag here.
>
>
> It doesn't make any sence to me to enable on-heap cache without evictions:
> it will result in having all data both in on-heap and off-heap memory. If
> we want to support such use case, we should implement a separate on-heap
> only mode with page memory disabled.
>
>
> On Tue, Apr 4, 2017 at 7:15 PM, Dmitriy Setrakyan <[hidden email]>
> wrote:
>
> > Ilya, I looked at the Semyon's comments in the ticket, and I think I
> agree
> > with him on all counts.
> >
> > 1. Setting an eviction policy should not be a mechanism to enable the
> > on-heap cache. We already have eviction policies off-heap as well, and
> they
> > don't enable anything. On top of that, the eviction policy should not be
> a
> > requirement for the on-heap cache. User should still be able to enable
> the
> > on-heap cache, even if it grows indefinitely without evictions. We should
> > have a more intuitive flag here.
> >
> > 2. As far as the tests go, they should examine all the tests and adapt
> them
> > to the new behavior. I think we should have a replica of all off-heap
> tests
> > to test the scenario with on-heap caches.
> >
> > D.
> >
> > On Tue, Apr 4, 2017 at 8:12 AM, Ilya Lantukh <[hidden email]>
> > wrote:
> >
> > > Hi Igniters,
> > >
> > > Since review of IGNITE-4535
> > > <https://issues.apache.org/jira/browse/IGNITE-4535> implementation
> > caused
> > > some misunderstandings, I'd like to open a discussion here and see if
> > > everyone agrees with the chosen approach or can suggest a better one.
> > >
> > > We are going to re-use existing EvictionPolicy mechanics to decide when
> > > entry is going to be evicted from on-heap cache. If evictionPolicy ==
> > null,
> > > we assume that there is no on-heap cache. One of suggested alternatives
> > was
> > > to have a separate boolean parameter that will enable on-heap cache.
> > >
> > > Another questionable decision was to remove tests for memory mode
> > > variations. For example, we had GridCacheContinuousQueryAtomic
> SelfTest,
> > > GridCacheContinuousQueryAtomicOffheapTieredSelfTest and
> > > GridCacheContinuousQueryAtomicOffheapValuesSelfTest that were testing
> > the
> > > same functionallity for ONHEAP_TIERED, OFFHEAP_TIERED and
> OFFHEAP_VALUES
> > > modes, respectively. Since those memory modes were removed, only
> > > GridCacheContinuousQueryAtomicSelfTest was left and it now runs in
> > > off-heap
> > > mode without on-heap cache. One of suggestions was to add a new
> subclass
> > to
> > > this test (and all other tests) that will run the same test case with
> > > on-heap cache enabled. In my opinion, functionallity that is specific
> for
> > > on-heap cache should be tested in completely separate tests (which we
> > > already have), and there is no need to run generic tests with every
> > > possible configuration.
> > >
> > > What do you think?
> > >
> > > --
> > > Best regards,
> > > Ilya
> > >
> >
>
>
>
> --
> Best regards,
> Ilya
>

[dsetrakyan]

On Wed, Apr 5, 2017 at 2:52 AM, Ilya Lantukh <[hidden email]> wrote:

> Dmitry,
>
> 1. Setting an eviction policy should not be a mechanism to enable the
> > on-heap cache. We already have eviction policies off-heap as well, and
> they
> > don't enable anything. On top of that, the eviction policy should not be
> a
> > requirement for the on-heap cache. User should still be able to enable
> the
> > on-heap cache, even if it grows indefinitely without evictions. We should
> > have a more intuitive flag here.
>
>
> It doesn't make any sence to me to enable on-heap cache without evictions:
> it will result in having all data both in on-heap and off-heap memory. If
> we want to support such use case, we should implement a separate on-heap
> only mode with page memory disabled.
>

I agree that this would cause a double memory consumption, but yet again,
we don't have a pure on-heap cache yet. Until we do, we should not require
an eviction policy present for the on-heap caches.