[infinispan-dev] ISPN-3557: interactions between a clear() operation and a Transaction

[Dan Berindei]

On Thu, Oct 3, 2013 at 1:57 PM, Sanne Grinovero <sanne at infinispan.org>wrote:

> On 3 October 2013 10:29, Dan Berindei <dan.berindei at gmail.com> wrote:
> >
> > On Wed, Oct 2, 2013 at 12:48 PM, Pedro Ruivo <pedro at infinispan.org>
> wrote:
> >>
> >>
> >>
> >> On 10/02/2013 12:03 AM, Sanne Grinovero wrote:
> >> > I'd love to brainstorm about the clear() operation and what it means
> >> > on Infinispan.
> >> >
> >> > I'm not sure to what extent, but it seems that clear() is designed to
> >> > work in a TX, or even create an implicit transaction if needed, but
> >> > I'm not understanding how that can work.
> >
> >
> > I think it's pretty straight-forward to define the semantics of
> operations
> > after clear(): they should assume that every key in the cache was
> removed.
> >
> > True, like Pedro mentioned, the current implementation allows subsequent
> > operations in the same tx to see keys inserted by other txs. But I don't
> > think fixing that would be harder than ensuring that if a tx inserts key
> k
> > owned by nodes A and B, clear() from outside the tx either deletes k on A
> > and B or doesn't delete k on either node.
> >
> >
> >>
> >> >
> >> > Obviously a clear() operation isn't listing all keys explicitly.
> >>
> >> (offtopic) like entrySet(), keySet(), values() and iterator() when
> >> executed in a transactional context. however, I think that this
> >> operations are more difficult to handle, because they have to return
> >> consistent data...
> >
> >
> > I agree, defining how entrySet() and the others should work in a tx seems
> > harder than defining clear().
>
> I agree, those other nasty beasts are next on my list: see below..
> But we're not arguing about which method deserves the gold medal for
> complex design ;-)
>
>
> > Will, I'm guessing your fix for ISPN-3560 still allows entrySet() after
> > clear() to see entries inserted by another tx in the meantime?
> >
> >>
> >> > Which
> >> > implies that it's undefined on which keys it's going to operate when
> >> > it's fired.. that seems like terribly wrong in a distributed key/value
> >> > store as we can't possibly freeze the global state and somehow define
> >> > a set of keys which are going to be affected, while an explicit
> >> > enumeration is needed to acquire the needed locks.
> >>
> >> you may not need to explicit acquire the lock for the affected key. you
> >> can use a global lock that is share acquired for write transactions
> >> without clear operation and exclusively acquire for transactions with
> >> clear operation (exactly what I use for Total Order). also, the clear
> >> transactions (i.e. the transactions with clear operation) can also only
> >> acquired the global lock.
> >
> >
> > +1 for the global lock. It may slow things down a bit in the general
> case,
> > because every tx will have to acquire it in shared mode, but it will
> > guarantee consistency for clear().
>
> I had thought of that too, but it will slow down all operations, at
> which benefit?
> To have a nice transactional clear() operation which guarantees
> consistency?
>

For me, the most important reason is to support the JCache API.


> The goal of this thread is to challenge if that makes sense for real use
> cases,
> it seems in fact we would highly prefer a non transactional version,
> and considering that we can't run a single Cache in both modes anymore
> that's a problem.
>

> Side note on the implementation complexity:
> even the AsyncCacheStore decorator code is made extremely more tricky
> just to handle clear() appropriately, to keep it in the correct order
> of execution
> compared to the other operations, while still allowing
> "coaleshing" of other operations, which essentially implies we allow
> reordering of operations as long as they affect independent keys.. where
> clear()
> is of course a jolly which requires it's own execution path, its own
> global lock
> and generally makes it far more complex.
>

Off-topic: I'm not very familiar with the async cache store code, but I
really don't think coalescing writes is good for consistency. In case of a
crash, you'd have lots of incomplete transactions on disk.


> >
> > It still won't help entrySet() and its brethren...
> >
> >>
> >> >
> >> > It might give a nice safe feeling that, when invoking a clear()
> >> > operation in a transaction, I can still abort the transaction to make
> >> > it cancel the operation; that's the only good part I can think of: we
> >> > can cancel it.
> >>
> >> yep
> >
> >
> > Wouldn't it be harder to integrate it with Total Order if it didn't have
> a
> > tx?
> >
> > But most of all, I think we have to keep it because it's a part of the
> Map
> > interface and a part of JCache's Cache interface.
> >
> >>
> >>
> >> >
> >> > I don't think it has anything to do with consistency though? To make
> >> > sure you're effectively involving all replicas of all entries in a
> >> > consistent way, a lock would need to be acquired on each affected key,
> >> > which again implies a need to enumerate all keys, including the
> >> > unknown keys which might be hiding in a CacheStore: it's not enough to
> >> > broadcast the clear() operation to all nodes and have them simply wipe
> >> > their local state as that's never going to deal correctly
> >> > (consistently) with in-flight transactions working on different nodes
> >> > at different times (I guess enabling Total Order could help but you'd
> >> > need to make it mandatory).
> >> >
> >>
> >> see the comment above about the locks... may not be the most efficient,
> >> but handles the problem.
> >
> >
> > Note that we need to iterate the keys anyway, in order to fire the
> > CacheEntryRemoved listeners. Even if we optimized it skip the iteration
> when
> > there are no registered listeners, it would still have to handle the
> general
> > case.
> >
> >>
> >> > So let's step back a second and consider what is the use case for
> >> > clear() ? I suspect it's primarily a method needed during testing, or
> >> > maybe cleanup before a backup is restored (operations), maybe a
> >> > manually activated JMX operation to clear the cache in exceptional
> >> > cases.
> >>
> >> I'm not aware of the uses cases of a clear() operation, but use it as
> >> JMX operation can be a problem in transactional caches (depending the
> >> semantic you have in mind).
> >>
> >> If it is going to clear all the caches (i.e. from all replicas/nodes),
> >> then we have to find a way to set an order with the current transactions
> >> or we will have something some nodes delivering a transaction (say tx1)
> >> before the clear() and other nodes delivering tx1 after the clear(),
> >> leading to an inconsistent state.
> >>
> >> (offtopic) I'm wondering if we have that problem in the current
> >> implementation if tx1 is only insert new keys... in that case, we have
> >> no lock to serialize an order between the clear() and tx1...
> >
> >
> > I'm pretty sure we do have this problem. We definitely need to add some
> > tests for clear concurrent with other write operations.
> >
> > In my view, as long as we're going to expose a clear() operation in some
> > way, it's going to be easiest to include it in a tx in transactional
> caches.
> > Exposing it in some other way is not going to make it any more
> consistent,
> > although prohibiting other operations in the same tx may make the code a
> bit
> > simpler.
> >
> >>
> >>
> >> >
> >> > I don't think there would ever be a need for a clear() operation to
> >> > interact with other transactions, so I'd rather make it illegal to
> >> > invoke a clear() inside a transaction, or simply ignore the
> >> > transactional scope and have an immediate and distributed effect.
> >
> >
> > We can't control whether other transactions are invoked concurrently with
> > clear(), so we can't make that illegal... If you mean interaction with
> other
> > operations in the same transaction, I agree that it's not absolutely
> > necessary to allow them, but I think most of that work has already been
> > done.
> >
> >>
> >> I think the second solution can make inconsistency data ("or simply
> >> ignore the transactional scope and have an immediate and distributed
> >> effect")
> >
> >
> > Maybe that's the idea? Sanne, do you mean that if clear() were exposed
> via a
> > different API, it wouldn't have to provide the same consistency
> guarantees
> > as regular operations?
>
> Yes that's the main point that we need to clarify.
> Note that this discussion started because of a problem we're having in
> the integration
> of Infinispan as Hibernate 2nd level cache, which is a quite mature
> contract with
> some very clear ideas of how a cache needs to behave.
>

> After more discussions yesterday, I think I'm understanding (Galder
> will likely know more
> but is currently away) that ideally a cache should be able to provide
> both a transactional
> clear() operation and an immediately-affecting one.
> The only need for the transactional one is to be able to undo the
> operation in case the
> user's transaction is rolled back: we don't even support this case
> today since it's suspending
> the transaction to run the clear() operation in an independent transaction.
>

> But the transactional clear() operation is far less important than
> having a clear "evict all now"
> operation. That's one of the basic operations a good mannered Cache
> should provide:
> remember that as a cache it's always a valid answer to return "null",
> it's just suboptimal.
> Returning the wrong answer instead, can have critical consequences, so
> it's expected to
> provide a way for the admin (JMX) or driving application to
> immediately wipe the state;
> I would argue that even a glocal lock's timing implications are
> undesirable in such a case.
>

TBH I'm not sure how would such an "evict all now" operation would work in
a distributed cache. Could you post a link to the Hibernate docs?
The JCache javadoc for clear() doesn't say anything about its interaction
with transactions.


>
>
> Focusing on the identify of Infinispan as a Key/Value store, I'd
> consider it almost mandatory
> for the client to know - to explicitly list - the keys it's interested
> into, and be completely oblivious
> to the entries which you're unable to list. For example, think of a
> cache shared by multiple tenants
> or multiple apps, which use keys which for some reason can't return
> true to an equality check
> (for example because they're coming from different classloaders).
> In such a perspective, any operation which affects (or enumerates)
> keys is a bad practice and
> should imho be banned from the user API, unless for administration or
> monitoring purposes.
>

Our Map/Reduce API explicitly supports the user not listing the keys he's
interested in.
And we don't support multi-tenancy in a single cache, every cache must have
a single ClassLoader.


>
> I think this is quite clearly expressed already by all the warning in
> javadoc on methods like
>   org.infinispan.Cache.keySet()
>   org.infinispan.Cache.size()
>   ...
> They all list a warning about needing to be used only for debug
> purposes: I don't think we need
> to waste time into making these transitionally consistent, as people
> shouldn't be using them.
>

The JCache API still has a clear() method and an iterator() method, even
though their Cache interface doesn't extend Map. That makes me think that
there may be legitimate uses for them, after all. And some users may never
see our javadocs, if they use the JCache API.


>
>
> If a transactional clear() operation is possible without overhead,
> then the 2nd level cache code
> would benefit from it, but it's a borderline decision and its code
> could be refactored to not need this,
> at least that's my understanding so far and I guess we need Galder for
> that to be clarified.
>
> Sanne
>
> >
> >>
> >>
> >> >
> >> > I'm likely missing something. What terrible consequences would this
> >> > have?
> >> >
> >> > Cheers,
> >> > Sanne
> >> > _______________________________________________
> >> > infinispan-dev mailing list
> >> > infinispan-dev at lists.jboss.org
> >> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
> >> >
> >> _______________________________________________
> >> infinispan-dev mailing list
> >> infinispan-dev at lists.jboss.org
> >> https://lists.jboss.org/mailman/listinfo/infinispan-dev
> >
> >
> >
> > _______________________________________________
> > infinispan-dev mailing list
> > infinispan-dev at lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/infinispan-dev/attachments/20131009/51f4a859/attachment-0001.html