[infinispan-dev] Write-only commands

[Adrian Nistor]

<<So SKIP_INDEXING should not skip indexing because it can break query??>>

Dan, drawing the conversation into absurdity is not useful.



On 06/29/2017 03:36 PM, Dan Berindei wrote:
> On Thu, Jun 29, 2017 at 2:19 PM, Radim Vansa <rvansa at redhat.com> wrote:
>> On 06/29/2017 11:16 AM, Dan Berindei wrote:
>>> On Thu, Jun 29, 2017 at 11:53 AM, Radim Vansa <rvansa at redhat.com> wrote:
>>>> On 06/28/2017 04:20 PM, Dan Berindei wrote:
>>>>> On Wed, Jun 28, 2017 at 2:17 PM, Radim Vansa <rvansa at redhat.com> wrote:
>>>>>> On 06/28/2017 10:40 AM, Dan Berindei wrote:
>>>>>>> On Wed, Jun 28, 2017 at 10:17 AM, Radim Vansa <rvansa at redhat.com> wrote:
>>>>>>>> On 06/27/2017 03:54 PM, Dan Berindei wrote:
>>>>>>>>> On Tue, Jun 27, 2017 at 2:43 PM, Adrian Nistor <anistor at redhat.com> 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.
>>> We could change the clustered listeners so that they're not based on
>>> the regular listeners... I've been pestering Will about this ever
>>> since the clustered listeners landed!
>>>
>>> But I should have been clearer: I didn't mean that the listeners on
>>> the backups should receive the previous value whenever we feel like
>>> it, I meant we should document and enforce that the previous value is
>>> only included in the event for listeners on the primary owner.
>>>>>> 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 at redhat.com> 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?
>>>>
>>> If you mean that we should remove the flags from the public API, I
>>> agree. If you mean we should just ignore them, then no, because most
>>> of the flags were added for internal components that really need their
>>> semantics.
>> We can't remove them from public API before Infinspan 10, and I think
>> that it will be a quite an unpopular step even after that. But until 10,
>> I think that the common agreement was to not break query, that is ignore
>> the flags. And make write-only reading.
>>
> So SKIP_INDEXING should not skip indexing because it can break query??
>
>> R.
>>
>>> Dan
>>>
>>>
>>>> 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 at lists.jboss.org
>>>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>>> --
>>>>>> Radim Vansa <rvansa at redhat.com>
>>>>>> JBoss Performance Team
>>>>>>
>>>>>> _______________________________________________
>>>>>> 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
>>>> --
>>>> Radim Vansa <rvansa at redhat.com>
>>>> JBoss Performance Team
>>>>
>>>> _______________________________________________
>>>> 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
>>
>> --
>> Radim Vansa <rvansa at redhat.com>
>> JBoss Performance Team
>>
>> _______________________________________________
>> 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