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. > 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? 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 <kare...@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 <kare...@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@lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Radim Vansa <rva...@redhat.com> JBoss Performance Team _______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev
