[infinispan-dev] to be a command, or not to be a command, that is the question

Mon May 15 04:59:33 EDT 2017

--
Galder Zamarreño
Infinispan, Red Hat

> On 9 May 2017, at 20:39, Radim Vansa <rvansa at redhat.com> wrote:
> 
> On 05/08/2017 09:58 AM, Galder Zamarreño wrote:
>> Hey Katia,
>> 
>> Sorry for delay replying back! I'm surprised there has not been more feedback. My position on this is well known around the team, so let me summarise it:
>> 
>> My feeling has always been that we have too many commands and we should reduce number of commands. Part of the functional map experiment was to show with a subset of commands, all sorts of front end operations could be exposed. So, I'm on Radim's side on this. By passing functions/lambdas, we get a lot of flexibility with very little cost. IOW, we can add more operations by just passing in different lambdas to existing commands.
>> 
>> However, it is true that having different front API methods that only differ in the lambda makes it initially hard to potentially do different things for each, but couldn't that be solved with some kind of enum?
>> 
>> Although enums are useful, they're a bit limited, e.g. don't take params, so  since you've done Scala before, maybe this could be solved with some Scala-like sealed trait for each front end operation type? I used something like a sealed trait for implementing a more flexible flag system for functional map API called org.infinispan.commons.api.functional.Param
> 
> Do I understand correctly that you're suggesting to add a enum to 
> ReadWriteKeyValueCommand that will say "behave like eval 
> (current)/compute*/merge"? How is that different from just wrapping the 
> 'user function' into adapting function (with registered externalizer == 
> marshalling to just 1-2 bytes)?
> 
> Handling such enum in interceptors is not better that having additional 
> visitX method. And not handling that does not allow you to apply 
> optimizations which Katia has named as reason #1 to have the separate 
> commands.

TBH, ideally I wouldn't like to have any enums at all since that defeats the purpouse of having commands that have transparent lambdas. The commands themselves, whether Read-Only, Read-Write, Write-Only should be enough distinction to do that what you need to do...

However, in real life, I'm not 100% sure if that'd be enough to do what we do... Maybe better than enums, there could be special lambda-bearing commands.

> 
>> The problem I have with adding more commands is the explosion that it provokes in terms of code, with all the required visit* method impls all over the place...etc.
>> 
>> I personally think that the lack of a more flexible command architecture is what has stopped us from adding front-end operations more quickly (e.g. counters, multi-maps...etc). IMO, working with generic commands that take lambdas is a way to strike a balance between adding front-end operations quickly and not resulting in a huge explosion of commands.
> 
> So your final verdict is -1 to separate commands?

Yeah.

However, I'd say that this is all semi-internal implementation detail and we can change relatively easily. So even if work has already been done using separate commands, we should be able to change that down the line.

I call semi-internal because since our interceptor stack is configurable by the user, an advanced user might some day add an interceptor that visits a certain command...

Cheers,

> 
> R.
> 
> PS: besides DRY, I vote for the use of functional commands is that it 
> would encourage us to fix the rest of the parts that might not be 
> working properly - e.g. QueryInterceptor was not updated with the 
> functional stuff (but QI is broken in more ways [1])
> 
> [1] https://issues.jboss.org/browse/ISPN-7806
> 
>> 
>> Cheers,
>> --
>> Galder Zamarreño
>> Infinispan, Red Hat
>> 
>>> On 20 Apr 2017, at 16:06, Katia Aresti <karesti at redhat.com> wrote:
>>> 
>>> Hi all
>>> 
>>> Well, nobody spoke, so I consider that everybody agrees that I can take a decision like a big girl by myself ! :)
>>> 
>>> I'm going to add 3 new commands, for merge, compute&computeIfPresent and computeIfAbsent. So I won't use the actual existing commands for the implementation : ReadWriteKeyCommand and ReadWriteKeyValueCommand even if I'm a DRY person and I love reusing code, I'm a KISS person too.
>>> 
>>> I tested the implementation using these functional commands and IMHO :
>>> - merge and compute methods worth their own commands, they are very useful and we might want to adjust/optimize them individually
>>> - there are some technical issues related to the TypeConverterDelegatingAdvancedCache that makes me modify these existing functional commands with some hacky code that, for me, should be kept in commands like merge or compute with the correct documentation. They don't belong to a generic command.
>>> - Functional API is experimental right now. It might be non experimental in the near future, but we might decide to move to another thing. The 3 commands are already "coded" in my branches (not everything reviewed yet but soon). If one day we decide to change/simplify or we find a nice way to get rid of commands with a more generic one, removing and simplifying should be less painful than adding commands for these methods.
>>> 
>>> That's all !
>>> 
>>> Cheers
>>> 
>>> Katia
>>> 
>>> 
>>> 
>>> On Wed, Apr 12, 2017 at 12:11 PM, Katia Aresti <karesti at redhat.com> wrote:
>>> Hi all,
>>> 
>>> As you might know I'm working since my arrival, among other things, on ISPN-5728 Jira [1], where the idea is to override the default ConcurrentMap methods that are missing in CacheImpl (merge, replaceAll, compute ... )
>>> 
>>> I've created a pull-request [2] for compute, computeIfAbsent and computeIfPresent methods, creating two new commands. By the way, I did the same thing for the merge method in a branch that I haven't pull requested yet.
>>> 
>>> There is an opposite view between Radim and Will concerning the implementation of these methods. To make it short :
>>> In one side Will considers compute/merge best implementation should be as a new Command (so what is already done)
>>> In the other side, Radim considers adding another command is not necessary as we could simple implement these methods using ReadWriteKeyCommand
>>> 
>>> The detailed discussion and arguments of both sides is on GitHub [2]
>>> 
>>> Before moving forward and making any choice by myself, I would like to hear your opinions. For the record, it doesn't bother me redoing everything if most people think like Radim because working on commands has helped me to learn and understand more about infinispan internals, so this hasn't been a waste of time for me.
>>> 
>>> Katia
>>> 
>>> [1] https://issues.jboss.org/browse/ISPN-5728
>>> [2] https://github.com/infinispan/infinispan/pull/5046
>>> 
>>> 
>>> _______________________________________________
>>> 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