On Fri, Aug 19, 2016 at 3:39 PM, Dan Berindei <dan.berin...@gmail.com> wrote:
> I just had a discussion with Adrian, and he reminded me that the key > partitioners don't yet work with any HotRod client. > > Assuming we'll have for HotRod key partitioners soon, > AffinityPartitioner still won't work with remote caches, where we want > to keep the key as a byte[]. Instead, we would need a key partitioner > that knows protobuf and can parse a single field from the object. > > So I'd like to withdraw my support for making AffinityPartitioner the > default :) > > +1 for me, I don't think it should be default. Currently the Hot Rod protocol is hardcoded to use a hash function to map keys to segments (byte field in the header to indicate hash function version to use), so C#, C++, JS clients would not be able to take advantage of the AffinityPartitioner. Another thing to consider is for query usage, mapping 1 segment to 1 index shard uses lots of resources, and I've been exploring ways of making this configurable [1], which potentially could require a different partitioner. That does not mean it should never be default, we can revisit this later. Gustavo [1] https://issues.jboss.org/browse/ISPN-6225 > Cheers > Dan > > > On Fri, Aug 19, 2016 at 5:20 PM, Dan Berindei <dan.berin...@gmail.com> > wrote: > > On Thu, Aug 18, 2016 at 6:12 PM, Sanne Grinovero <sa...@infinispan.org> > wrote: > >> On 18 August 2016 at 14:44, Dan Berindei <dan.berin...@gmail.com> > wrote: > >>> First, a question: doesn't query already configure its internal caches > >>> to use the AffinityPartitioner? Does it need the application caches to > >>> have AffinityPartitioner enabled, too? > >> > >> No, we may provide reasonable defaults so that demos and tutorials > >> work out of the box but the general recommendation is that people make > >> their own configuration, as they will need to tune things, or add > >> CacheStores, etc.. > >> > > > > I see... I wonder if we could at least have some templates for the > > user to extend, without having to copy a bunch of stuff from > > tutorials? > > > > > >>> I'm ok with making AffinityPartitioner the default key partitioner: > >>> the less configuration the user has to change to make things work, the > >>> better. I'm also ok with changing it to use delegation, to make it > >>> easy for user to compose it with another key partitioner. We should > >>> just run some benchmarks first to check it doesn't affect performance > >>> for repl mode reads. > >> > >> Ok! > >> https://issues.jboss.org/browse/ISPN-6956 > >> > >> I'll send a PR; regarding benchmarking I'll take the optimistic path: > >> we'll test it after it's integrated, worst case we can take it out. > >> > >>> But I feel users should be able to disable it if they want to, so I > >>> wouldn't always wrap user partitioners. Also, usually less magic == > >>> better. Query can validate the configuration and warn the user if the > >>> configuration doesn't have AffinityPartitioner in place. > >> > >> I disagree on this as this would require more options, which make it > >> harder to use, and there's no good enough reason to give away the > >> option to disable this; especially as it kills other features. > >> > > > > Well, grouping isn't enabled by default, even though that means > > AdvancedCache.getGroup(String) doesn't work with the default > > configuration. Sure, AffinityPartitioner's overhead is probably lower > > than GroupingPartitioner's for non-targeted keys, but would you want > > to wrap all key partitioners with AffinityPartitioner if we were > > already wrapping them all with GroupingPartitioner? > > > > TBH I don't know why a user would want to customize the key > > partitioner when he could implement AffinityTaggedKey instead... > > > > > >> BTW I never mentioned Query :) > > > > You did mention your "evil" plans to improve query performance :D > > > > > >> We happen to use this functionality in Query but as I mentioned other > >> teams are looking forward to use this functionality, so I think we > >> should promote this as a public API, a new feature of Infinispan core. > >> > > > > AffinityTaggedKey was already in the public API, even before > > AffinityPartitioner was the default. So now we have these options for > > influencing the key distribution: > > * Implementing AffinityTaggedKey > > * Implementing a custom KeyPartitioner > > * Grouping: annotating a key method with @Group, or implementing Grouper > > * Generating random keys and mapping them to nodes with > KeyAffinityService > > > > > >> So since you want to be able to disable it I won't wrap all user > >> configured implementations in my PR for ISPN-6956, but I'd prefer it > >> if as a second step you would reconsider and allow me to wrap them > >> all. > >> > > > > Feel free to open a separate PR for this, I'm definitely not going to > > block it from going in. > > > > Cheers > > Dan > _______________________________________________ > 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
