Quantcast

[infinispan-dev] Switch to AffinityPartitioner by default, or even enforce it

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[infinispan-dev] Switch to AffinityPartitioner by default, or even enforce it

Sanne Grinovero-3
Hi all,

I would like for Infinispan to use the AffinityPartitioner by default,
replacing the HashFunctionPartitioner.

This should have zero impact as AffinityPartitioner extends
HashFunctionPartitioner and only changes semantics when a key is
implementing AffinityTaggedKey.

So the difference would be that for those using AffinityTaggedKey,
these would work out of the box without having to change the
configuration as well.

WDYT?

As a further but separate improvement, I'd change the
AffinityPartitioner to use delegation to the HashFunctionPartitioner
instead of extending it, and always wrap any user configured
partitioner with the AffinityPartitioner.
This would ensure that AffinityTaggedKey work as expectedf even when
people experiment with other partitioners, and avoids some complexity
of configuring Infinispan:

"oh, I didn't know that changing hashing function would break feature [x]..."

For the record, we're using AffinityTaggedKey in our evil plans to
improve query performance, but it's also sparked interest as a very
useful function by the HACEP team.

I have patches ready...

Thanks,
Sanne
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Switch to AffinityPartitioner by default, or even enforce it

Dan Berindei
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?

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.

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.

Cheers
Dan


On Thu, Aug 18, 2016 at 3:43 PM, Sanne Grinovero <[hidden email]> wrote:

> Hi all,
>
> I would like for Infinispan to use the AffinityPartitioner by default,
> replacing the HashFunctionPartitioner.
>
> This should have zero impact as AffinityPartitioner extends
> HashFunctionPartitioner and only changes semantics when a key is
> implementing AffinityTaggedKey.
>
> So the difference would be that for those using AffinityTaggedKey,
> these would work out of the box without having to change the
> configuration as well.
>
> WDYT?
>
> As a further but separate improvement, I'd change the
> AffinityPartitioner to use delegation to the HashFunctionPartitioner
> instead of extending it, and always wrap any user configured
> partitioner with the AffinityPartitioner.
> This would ensure that AffinityTaggedKey work as expectedf even when
> people experiment with other partitioners, and avoids some complexity
> of configuring Infinispan:
>
> "oh, I didn't know that changing hashing function would break feature [x]..."
>
> For the record, we're using AffinityTaggedKey in our evil plans to
> improve query performance, but it's also sparked interest as a very
> useful function by the HACEP team.
>
> I have patches ready...
>
> Thanks,
> Sanne
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Switch to AffinityPartitioner by default, or even enforce it

Sanne Grinovero-3
On 18 August 2016 at 14:44, Dan Berindei <[hidden email]> 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'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.

BTW I never mentioned Query :)
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.

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.

Thanks!
Sanne

>
> Cheers
> Dan
>
>
> On Thu, Aug 18, 2016 at 3:43 PM, Sanne Grinovero <[hidden email]> wrote:
>> Hi all,
>>
>> I would like for Infinispan to use the AffinityPartitioner by default,
>> replacing the HashFunctionPartitioner.
>>
>> This should have zero impact as AffinityPartitioner extends
>> HashFunctionPartitioner and only changes semantics when a key is
>> implementing AffinityTaggedKey.
>>
>> So the difference would be that for those using AffinityTaggedKey,
>> these would work out of the box without having to change the
>> configuration as well.
>>
>> WDYT?
>>
>> As a further but separate improvement, I'd change the
>> AffinityPartitioner to use delegation to the HashFunctionPartitioner
>> instead of extending it, and always wrap any user configured
>> partitioner with the AffinityPartitioner.
>> This would ensure that AffinityTaggedKey work as expectedf even when
>> people experiment with other partitioners, and avoids some complexity
>> of configuring Infinispan:
>>
>> "oh, I didn't know that changing hashing function would break feature [x]..."
>>
>> For the record, we're using AffinityTaggedKey in our evil plans to
>> improve query performance, but it's also sparked interest as a very
>> useful function by the HACEP team.
>>
>> I have patches ready...
>>
>> Thanks,
>> Sanne
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Switch to AffinityPartitioner by default, or even enforce it

Dan Berindei
On Thu, Aug 18, 2016 at 6:12 PM, Sanne Grinovero <[hidden email]> wrote:

> On 18 August 2016 at 14:44, Dan Berindei <[hidden email]> 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
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Switch to AffinityPartitioner by default, or even enforce it

Dan Berindei
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 :)

Cheers
Dan


On Fri, Aug 19, 2016 at 5:20 PM, Dan Berindei <[hidden email]> wrote:

> On Thu, Aug 18, 2016 at 6:12 PM, Sanne Grinovero <[hidden email]> wrote:
>> On 18 August 2016 at 14:44, Dan Berindei <[hidden email]> 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
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Switch to AffinityPartitioner by default, or even enforce it

Sanne Grinovero-3
On 19 August 2016 at 15:39, Dan Berindei <[hidden email]> wrote:
> I just had a discussion with Adrian, and he reminded me that the key
> partitioners don't yet work with any HotRod client.

In Hot Rod "out of the box" absolutely nothing works, as you have to set
key-equivalence="org.infinispan.commons.equivalence.ByteArrayEquivalence"
at the very least just to be able to get the "Get" operation working.

Which is another thing I don't like, but probably unrelated as it
looks like we epxect people to use our "server configurations" which
will have different settings.

>
> 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 :)
>
> Cheers
> Dan
>
>
> On Fri, Aug 19, 2016 at 5:20 PM, Dan Berindei <[hidden email]> wrote:
>> On Thu, Aug 18, 2016 at 6:12 PM, Sanne Grinovero <[hidden email]> wrote:
>>> On 18 August 2016 at 14:44, Dan Berindei <[hidden email]> 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
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Switch to AffinityPartitioner by default, or even enforce it

Gustavo Fernandes-2


On Fri, Aug 19, 2016 at 3:44 PM, Sanne Grinovero <[hidden email]> wrote:
On 19 August 2016 at 15:39, Dan Berindei <[hidden email]> wrote:
> I just had a discussion with Adrian, and he reminded me that the key
> partitioners don't yet work with any HotRod client.

In Hot Rod "out of the box" absolutely nothing works, as you have to set
key-equivalence="org.infinispan.commons.equivalence.ByteArrayEquivalence"
at the very least just to be able to get the "Get" operation working.


This is set for you via inheritance at [1], it won't work if using the HotServer class to start the server in unit tests for example.


[1] https://github.com/infinispan/infinispan/blob/master/server/integration/infinispan/src/main/resources/infinispan-defaults.xml

Gustavo
 

Which is another thing I don't like, but probably unrelated as it
looks like we epxect people to use our "server configurations" which
will have different settings.

>
> 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 :)
>
> Cheers
> Dan
>
>
> On Fri, Aug 19, 2016 at 5:20 PM, Dan Berindei <[hidden email]> wrote:
>> On Thu, Aug 18, 2016 at 6:12 PM, Sanne Grinovero <[hidden email]> wrote:
>>> On 18 August 2016 at 14:44, Dan Berindei <[hidden email]> 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
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Switch to AffinityPartitioner by default, or even enforce it

Dan Berindei
In reply to this post by Sanne Grinovero-3
Is there any way that AffinityPartitioner could work with HotRod, though?


On Fri, Aug 19, 2016 at 5:44 PM, Sanne Grinovero <[hidden email]> wrote:

> On 19 August 2016 at 15:39, Dan Berindei <[hidden email]> wrote:
>> I just had a discussion with Adrian, and he reminded me that the key
>> partitioners don't yet work with any HotRod client.
>
> In Hot Rod "out of the box" absolutely nothing works, as you have to set
> key-equivalence="org.infinispan.commons.equivalence.ByteArrayEquivalence"
> at the very least just to be able to get the "Get" operation working.
>
> Which is another thing I don't like, but probably unrelated as it
> looks like we epxect people to use our "server configurations" which
> will have different settings.
>
>>
>> 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 :)
>>
>> Cheers
>> Dan
>>
>>
>> On Fri, Aug 19, 2016 at 5:20 PM, Dan Berindei <[hidden email]> wrote:
>>> On Thu, Aug 18, 2016 at 6:12 PM, Sanne Grinovero <[hidden email]> wrote:
>>>> On 18 August 2016 at 14:44, Dan Berindei <[hidden email]> 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
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Switch to AffinityPartitioner by default, or even enforce it

Gustavo Fernandes-2
In reply to this post by Dan Berindei
On Fri, Aug 19, 2016 at 3:39 PM, Dan Berindei <[hidden email]> 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

 
Cheers
Dan


On Fri, Aug 19, 2016 at 5:20 PM, Dan Berindei <[hidden email]> wrote:
> On Thu, Aug 18, 2016 at 6:12 PM, Sanne Grinovero <[hidden email]> wrote:
>> On 18 August 2016 at 14:44, Dan Berindei <[hidden email]> 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
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Loading...