CacheStore and CacheStoreAdapter questions.

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

Folks,

I do not understand why CacheStore is a class and not an interface, and why
do we have another abstract class CacheStoreAdapter.

As I see that CacheStore have two non-abstract methods:

   - ignite() - gives access to injected Ignite instance. Used only in
   JdbcCacheStore.
   - session() - gives access to store session for TX cache. Used in dozens
   places. It returns private "ses" field. This field is not annotated anyhow
   and as I understand it is set somewhere in Ignite internals through
   reflection.

Both methods can be refactored as follows:

   - ignite() must be removed. Protected annotated field with Ignite
   instance must be added to JdbcCacheStore instead.
   - session() method must be made abstract and another abstract method
   session(CacheStoreSession) must be added. This way, we will set session
   through interface method call instead of hard-coded reflective field update.

This way we will be able to convert CacheStore to interface with minimum
changes. It will significantly improve Ignite store extensibility (e.g. we
already had to replace inheritance with composition for GridGain interop
store what made code bigger and dirtier).

Vladimir.

Re: CacheStore and CacheStoreAdapter questions.

[Semyon Boikov <sb...@gridgain.com>]

Initially I implemented CacheStore as interface and added annotation
CacheStoreSessionResource so
that session could be injected if needed. Then after review we decided to
change it to abstract class, but we can change it back to interface and
CacheStoreSessionResource.

On Thu, Feb 5, 2015 at 8:51 PM, Dmitriy Setrakyan <ds...@apache.org>
wrote:

> Vova,
>
> I agree that Ignite implementation can be injected in a concrete subclass.
> However, I disagree with adding setter for CacheStoreSession.  Having
> setters on an interface looks a bit weird to me.
>
> I believe we should either have an abstract class, or create
> CacheStoreSessionResource and auto-inject it.
>
> Thoughts? (Semyon, given that you have created the CacheStore class, what
> do you think?)
>
> D.
>
> On Thu, Feb 5, 2015 at 2:32 AM, Vladimir Ozerov <vo...@gridgain.com>
> wrote:
>
> > Folks,
> >
> > I do not understand why CacheStore is a class and not an interface, and
> why
> > do we have another abstract class CacheStoreAdapter.
> >
> > As I see that CacheStore have two non-abstract methods:
> >
> >    - ignite() - gives access to injected Ignite instance. Used only in
> >    JdbcCacheStore.
> >    - session() - gives access to store session for TX cache. Used in
> dozens
> >    places. It returns private "ses" field. This field is not annotated
> > anyhow
> >    and as I understand it is set somewhere in Ignite internals through
> >    reflection.
> >
> > Both methods can be refactored as follows:
> >
> >    - ignite() must be removed. Protected annotated field with Ignite
> >    instance must be added to JdbcCacheStore instead.
> >    - session() method must be made abstract and another abstract method
> >    session(CacheStoreSession) must be added. This way, we will set
> session
> >    through interface method call instead of hard-coded reflective field
> > update.
> >
> > This way we will be able to convert CacheStore to interface with minimum
> > changes. It will significantly improve Ignite store extensibility (e.g.
> we
> > already had to replace inheritance with composition for GridGain interop
> > store what made code bigger and dirtier).
> >
> > Vladimir.
> >
>

Re: CacheStore and CacheStoreAdapter questions.

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

Vova,

I agree that Ignite implementation can be injected in a concrete subclass.
However, I disagree with adding setter for CacheStoreSession.  Having
setters on an interface looks a bit weird to me.

I believe we should either have an abstract class, or create
CacheStoreSessionResource and auto-inject it.

Thoughts? (Semyon, given that you have created the CacheStore class, what
do you think?)

D.

On Thu, Feb 5, 2015 at 2:32 AM, Vladimir Ozerov <vo...@gridgain.com>
wrote:

> Folks,
>
> I do not understand why CacheStore is a class and not an interface, and why
> do we have another abstract class CacheStoreAdapter.
>
> As I see that CacheStore have two non-abstract methods:
>
>    - ignite() - gives access to injected Ignite instance. Used only in
>    JdbcCacheStore.
>    - session() - gives access to store session for TX cache. Used in dozens
>    places. It returns private "ses" field. This field is not annotated
> anyhow
>    and as I understand it is set somewhere in Ignite internals through
>    reflection.
>
> Both methods can be refactored as follows:
>
>    - ignite() must be removed. Protected annotated field with Ignite
>    instance must be added to JdbcCacheStore instead.
>    - session() method must be made abstract and another abstract method
>    session(CacheStoreSession) must be added. This way, we will set session
>    through interface method call instead of hard-coded reflective field
> update.
>
> This way we will be able to convert CacheStore to interface with minimum
> changes. It will significantly improve Ignite store extensibility (e.g. we
> already had to replace inheritance with composition for GridGain interop
> store what made code bigger and dirtier).
>
> Vladimir.
>