Re: [infinispan-dev] Write-only commands

On 06/28/2017 04:20 PM, Dan Berindei wrote: > On Wed, Jun 28, 2017 at 2:17 PM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >> On 06/28/2017 10:40 AM, Dan Berindei wrote: >>> On Wed, Jun 28, 2017 at 10:17 AM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >>>> On 06/27/2017 03:54 PM, Dan Berindei wrote: >>>>> On Tue, Jun 27, 2017 at 2:43 PM, Adrian Nistor <anistor(a)redhat.com&gt; wrote: >>>>>> I've said this in a previous thread on this same issue, I will repeat myself >>>>>> as many times as needed. >>>>>> >>>>>> Continuous queries require the previous value itself, not just knowledge of >>>>>> the type of the previous value. Strongly typed caches solve no problem here. >>>>>> >>>>>> So if we half-fix query but leave CQ broken I will be half-happy (ie. very >>>>>> depressed) :) >>>>>> >>>>>> I'd remove these commands completely or possibly remove them just from >>>>>> public API and keep them internal. >>>>>> >>>>> +1 to remove the flags from the public API. Most of them are not safe >>>>> for applications to use, and ignoring them when they can lead to >>>>> inconsistencies would make them useless. >>>>> >>>>> E.g. the whole point of SKIP_INDEX_CLEANUP is that the cache doesn't >>>>> know when it is safe to skip the delete statement, and it relies on >>>>> the application making a (possibly wrong) choice. >>>>> >>>>> IGNORE_RETURN_VALUES should be safe to use, and we actually recommend >>>>> that applications use it right now. If query or listeners need the >>>>> previous value, then we should load it internally, but hide it from >>>>> the user. >>>>> >>>>> But removing it opens another discussion: should we replace it in the >>>>> public API with a new method AdvancedCache.ignoreReturnValues(), or >>>>> should we make it the default and add a method >>>>> AdvancedCache.forceReturnPreviousValues()? >>>> Please don't derail the thread. >>>> >>> I don't think I'm derailing the thread: IGNORE_PREVIOUS_VALUES also >>> breaks the previous value for listeners, even if the QueryInterceptor >>> removes it from write commands. And it is public (+recommended) API, >>> in fact most if not all of our performance tests use it. >> That's just a flawed implementation. IPV is documented to be a 'safe' >> flag that should affect mostly primary -> origin replication, all the >> other is implementation. And we can fix that. Users should *not* expect >> that it e.g. skips loading from a cache store. We have already removed >> the modes that would be broken-by-design. >> > I think you're confusing IGNORE_RETURN_VALUES with SKIP_REMOTE_LOOKUP > here. The IVR javadoc doesn't say anything about remote lookups, only > SRL does. No, I am not; While IRV does not mention the replication, it's said to be 'safe'. So omitting the primary -> origin replication is basically all it can do when listeners are in place. You're right that I have missed the second part in SRL talking about put()s; I took it as a flag prohibiting any remote lookup (as the RPC operation in its whole) any time the remote value is needed. Yes, the second part seems equal to my understanding of IRV. > > And I agree that the current status is far from ideal, but there is > one more valid alternative: we can decide that the previous value is > only reliable in clustered listeners, and local listeners don't always > have it. Document that, make sure continuous query uses clustered > listeners, and we're done :) Unreliable return values are worse than none; I would rather remove them if we can't guarantee that these are right. Though, clustered listeners are based on regular listeners, so you'd need some means to make them reliable.

>> On the other hand, write-only commands are not about *returning* the >> value but about (not) *reading* it, therefore (in my eyes) user could >> make that assumption and would like to enforce it this way. Even some >> docs explaining PersistenceMode.SKIP suggest that. >> > To me the purpose the same, there is no difference between returning > the previous value to the application or providing the previous value > via EntryView. There is a difference between what's provided locally and what's send over the network. > Applying this logic to the JCache API, it would mean > put() should never read the previous value, because some users could > assume that only getAndPut() reads it. OK, this is a valid point. > > In the old times we didn't have IGNORE_RETURN_VALUES, only > SKIP_REMOTE_LOOKUP+SKIP_CACHE_LOAD, and they would sometimes be > ignored (e.g. if the write was conditional). I think that's what > Galder had in mind when he wrote the PersistenceMode api note, not the > current behaviour of SKIP_CACHE_LOAD. I'll let Galder clarify this > himself, but I'll be very disappointed if he says he designed the > write-only operations so that they'll never work with query. > > >> I don't want to talk about flags, because I see all flags but IPV as >> 'effectively internal'. Let's discuss it more high-level. Some API >> exposes non-reading operation - we can see that under some circumstances >> this is not possible so we have options to 1) break stuff 2) break API >> assumptions 3) sometimes break API assumptions 4) remove such API (to >> not allow the user to make such assumptions). There's also an option 5) >> to fail the operation if the API assumption would be broken. Though, I >> don't fancy getting exception from a WriteOnlyMap.eval just because >> someone has registered a listener. >> > I disagree with the premise: there's no good reason for the user to > assume that write-only commands are *guaranteed* to never load the > previous value from a store. We just need to add a clarification to > the write-only operations' javadoc, no need to break anything. OK then, though it diminishes the value of write-only commands a lot. > > >>> For that matter, ClusteredCacheLoaderInterceptor also doesn't load the >>> previous value on backup owners for most write commands >>> (LoadType.PRIMARY), we'd need to change that as well. >> Yes, all commands will have to load current value on all owners. >> >>>>>> On 06/27/2017 01:28 PM, Sanne Grinovero wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 27 Jun 2017 10:13, "Radim Vansa" <rvansa(a)redhat.com&gt; wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> I am working on entry version history (again). In Como we've discussed >>>>>> that previous values are needed for (continuous) query and reliable >>>>>> listeners, >>>>>> >>>>>> >>>>>> Index based queries also require the previous value on a write - unless we >>>>>> can get "strongly typed caches" giving guarantees about the class to >>>>>> represent the content of a cache to be unique. >>>>>> >>>>>> Essentially we only need to know the type of the previous object. It might >>>>>> be worth having a way to load the type metadata if the previous value only. >>>>>> >>>>>> so I wonder what should we do with functional write-only >>>>>> commands. These are different to commands with flags, because flags >>>>>> (other than ignore return value) are expected to break something. >>>>>> >>>>>> >>>>>> Sorry I hope to not derail the thread but let's remind that we hope to >>>>>> evolve beyond "flags are expected to break stuff" ; we never got to it but >>>>>> search the mailing list. >>>>>> >>>>>> Since flags are exposed to the user I would rather they're not allowed to >>>>>> break things. >>>>>> Could they be treated as hints? Ignore the flag (and warn?) if the used >>>>>> configuration/integrations veto them. >>>>>> >>>>>> Alternatively, let's remove them from API. Remember "The Jokre" POC was >>>>>> intentionally designed to explore pushing the limits on performance w/o end >>>>>> users having to solve puzzles, such as learning details about these flags >>>>>> and their possible side effects. >>>>>> >>>>>> So assuming they become either "safe" or internal, maybe you can take >>>>>> advantage of them? >>>>>> >>>>>> I see >>>>>> the available options as: >>>>>> >>>>>> 1) run write-only commands 'optimized', ignoring any querying and such >>>>>> (warn user that he will break it) >>>>>> >>>>>> 2) run write-only without any optimization, rendering them useless >>>>>> >>>>>> 3) detect when querying is set up (ignoring listeners and maybe other >>>>>> stuff that could get broken) >>>>>> >>>>>> >>>>>> Might be useful for making a POC work, but I believe query will be very >>>>>> likely to be often enabled. >>>>>> Having an either / or switch for different features in Infinispan will make >>>>>> it harder to use and understand, so I'd rather see work on the right design >>>>>> as taking temporary shortcuts risks baking into stone features which we >>>>>> later struggle to fix or maintain. >>>>>> >>>>> I vote for this option. >>>>> >>>>> Query, listeners, and other components that need the previous value >>>>> should not just assume that the application knows better, they should >>>>> be able to change how operations works based on their needs. Of >>>>> course, the reverse is also true: if the application uses write-only >>>>> commands (or IGNORE_RETURN_VALUES) for performance reasons, it should >>>>> be possible for the user to detect why the previous values are still >>>>> loaded. >>>> If it were just query (static configuration), I would be okay with this >>>> idea. But as per listeners - besides tainting the design (event source >>>> should not check if there's a listener) you'd need to check *before* >>> The source wouldn't check for listeners explicitly, the notifier would >>> have an isPreviousValueNeeded() method and precompute that before a >>> listener is added or after a listener is removed. I was am assuming >>> some listeners will not need the previous value, e.g. the listeners >>> installed by streams. >> You can cover your warts with a make-up but you'll still have warts :) > Cutting them off doesn't necessarily work, either :) Yep, some people tend to fix w/ hacks instead of designing :) > >>>> (DistributionI, CacheLoaderI) you have to call notify (cmd.perform, >>>> EWI). So this is a space for race conditions or weird handling (if >>>> there's a listener when I am about to call notify and my flags are not >>>> cleared, skip the notification and pretend that this code was invoked >>>> before the listener was registered...). Or do you have another solution >>>> in mind (config option to disable listeners && all features using those?). >>>> >>> I was definitely going for the weird handling... >>> >>> My plan was to set a HAS_PREVIOUS_VALUE flag on the context entry when >>> it's loaded, and check that before invoking a listener that needs the >>> previous value. It is missing one edge case: if one thread starts a >>> write operation, then another thread installs a listener that requires >>> the previous value and iterates over the cache, the second thread may >>> not see the value written by the first thread. >> If the operations overlap, you could pretend that the write has finished >> before the listener was invoked and simply not notify the listener. If I >> am missing it please write it down in code. But handling this in any way >> is still clumsy. > I hope pseudo-code is fine... > > 1. cache.put(k, v1) starts, doesn't load the previous value v0 in the context > 2. cache.addListener(l) runs, doesn't block > 3. cache.entrySet().forEach() runs, finds k->v0 > 4. cache.put(k, v1) commits k->v1, should notify the listener but > doesn't have the previous value > 5. cache.put(k, v0) returns, but the code that installed the listener > thinks the value of k is still v0 Oh OK, I should have drawn that myself when considering the scenario. You're right, here we'll have to retry. All in all, I think this discussion is done. We'll tell users to stick their flags where the sun doesn't shine and remove any inconvenient ones. Should we issue a warning any time we're removing the flag?

Radim > > >>> So now I'm thinking we should retry the write commands when >>> isPreviousValueNeeded() changes... Not very appealing, but I think the >>> performance difference is worth it. >>> >>>> R. >>>> >>>>>> 4) remove write-only commands completely (and probably functional >>>>>> listeners as well because these will lose their purpose) >>>>>> >>>>>> >>>>>> +1 to remove "unconditional writes", at least an entry version check should >>>>>> be applied. >>>>>> I believe we had already pointed out this would eventually happen, pretty >>>>>> much for the reasons you're hitting now. >>>>>> >>>>> IMO version checks should be done internally, we shouldn't force the >>>>> users of the functional API to deal with versions themselves because >>>>> we know how hard making write skew checks work is for us :) >>>>> >>>>> And I wouldn't go as far as to remove the functional listeners, >>>>> instead I would change them so that read-write listeners are invoked >>>>> on write-only operations and they force the loading of the previous >>>>> value. I would also add a way for the regular listeners to say whether >>>>> they need the previous value or not. >>>>> >>>>>> Right now I am inclined towards 4). There could be some internal use >>>>>> (e.g. multimaps) that could use 1) which is ran without a fancy setup, >>>>>> though, but it's asking for trouble. >>>>>> >>>>>> >>>>>> I agree! >>>>>> >>>>>> Thanks >>>>>> >>>>>> >>>>>> WDYT? >>>>>> >>>>>> Radim >>>>>> >>>>> Cheers >>>>> Dan >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> -- >> Radim Vansa <rvansa(a)redhat.com&gt; >> JBoss Performance Team >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Radim Vansa <rvansa(a)redhat.com&gt; JBoss Performance Team _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev