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