Hello, Pavel.
I support your proposal to transparently create custom AffinityMapper based on
server node classname, in general.
Partition Awareness crucial point for a thin client performance so It should be
absolutely clear for a user - PA works correctly or not.
So, I think, we should implement the following:
0. Add ability to sent custom affinity class name.
1. If `partitionAwarenessEnabled=true` but custom affinity can’t be
instantiated on the client - throw an exception.
WDYT?
> 8 июля 2022 г., в 08:37, Pavel Tupitsyn <ptupit...@apache.org> написал(а):
>
> To clarify: you can use a different AffinityFunction implementation on the
> client side. It just has to map to the same binary typeId.
> Even if there is a legacy setup with AffinityKeyMapper on servers, you can
> craft an AffinityFunction for the client that achieves correct behavior.
> So I believe this should cover any use case.
>
> On Thu, Jul 7, 2022 at 10:36 PM Pavel Tupitsyn <ptupit...@apache.org> wrote:
>
>> AffinityKeyMapper is just a subset of AffinityFunction, isn't it?
>> So by supporting AffinityFunction we cover all AffinityKeyMapper use cases.
>>
>>
>>> managing the classpath, a user should always keep in mind it
>> I don't consider this a problem. This is how Java works, what can we do?
>> You can also stick the class into BinaryConfiguration on the client to
>> make it explicit and make sure it is loaded.
>>
>>> it still possible having user bad implementations of AffinityFunction
>> that can't be easily instantiated;
>> It is also possible to provide a bad implementation of ToIntFunction, no
>> difference.
>>
>>> a question - what we should do in case of class instantiating loading
>> fails?
>> Currently we don't fail on custom affinity, right? So we should keep this
>> behavior. Print a warning.
>>
>> On Thu, Jul 7, 2022 at 10:27 PM Maxim Muzafarov <mmu...@apache.org> wrote:
>>
>>> Pavel,
>>>
>>>
>>> Yes, you're right that AffinityKeyMapper is deprecated, this is sad to
>>> say but it still used.
>>>
>>> Let me describe the cases in more detail. In general, we have
>>> customers which are using:
>>>
>>> 1. an own custom AffinityFunction;
>>> 2. the default RendezvousAffinityFunction + an own deprecated
>>> AffinityKeyMapper;
>>> 3. an own custom AffinityFunction + an own deprecated
>>> AffinityKeyMapper (I doubt about this case);
>>>
>>> I'd like to provide a general solution that solves all the cases
>>> mentioned above, thus we can't discuss only AffinityFunction. It
>>> doesn't fit the initial goals.
>>>
>>>
>>>> Can you please clarify the "tricky" part?
>>>
>>> Yes, from my point of view the tricky part here is:
>>>
>>> - managing the classpath, a user should always keep in mind it;
>>> - it still possible having user bad implementations of
>>> AffinityFunction that can't be easily instantiated;
>>> - a question - what we should do in case of class instantiating
>>> loading fails? drop the client, print the warn or do nothing. These
>>> solutions have a different impact on a production environment and user
>>> may have different expectations on that;
>>>
>>>
>>> I'm not saying that solution you suggested is bad, it just has some
>>> drawbacks (like other solutions) and doesn't solves all the cases if
>>> we're not instantiating a deprecated AffinityKeyMapper (which looks a
>>> bit ugly for me too).
>>>
>>>
>>> Actually, we are not limited with setting/instantiating
>>> AffinityFunction. We can provide for a user a control on which channel
>>> (node) a particular cache request will be executed (this thing also
>>> have a drawback - a transactional operation cannot be executed on an
>>> arbitrary node, it should be executed on node it's started).
>>>
>>> On Thu, 7 Jul 2022 at 21:41, Pavel Tupitsyn <ptupit...@apache.org> wrote:
>>>>
>>>>> AffinityKeyMapper (it will be required too)
>>>> It is deprecated. We should not add any functionality on top of
>>> deprecated
>>>> stuff.
>>>> Let's discuss only AffinityFunction.
>>>>
>>>>> this approach requires of these classes to be present on a thin client
>>>> side and it can be tricky
>>>> Any of the approaches discussed above requires some additional logic to
>>> be
>>>> present on the client.
>>>> Can you please clarify the "tricky" part?
>>>>
>>>> All you need is to provide AffinityFunction implementation with a proper
>>>> class name
>>>> (or even use nameMapper/idMapper to map to a different name), it is no
>>>> different from the ToIntFunction approach.
>>>>
>>>>
>>>> On Thu, Jul 7, 2022 at 9:23 PM Maxim Muzafarov <mmu...@apache.org>
>>> wrote:
>>>>
>>>>> Folks,
>>>>>
>>>>> I'd like also to mention since we are talking about a customer's
>>>>> custom affinity function and affinity mapper implementations we could
>>>>> face with some issues during these classes instantiation. It could
>>>>> work for the customer which I've faced on, but I cannoun be sure that
>>>>> this approach will work for all of the cases.
>>>>>
>>>>> I think it is better to avoid such an issues and leave it up to a
>>>>> user. Nevertheless, the worst scenario for the user would be having 2
>>>>> hops on each put/get as it was before this thread.
>>>>>
>>>>> On Thu, 7 Jul 2022 at 21:12, Maxim Muzafarov <mmu...@apache.org>
>>> wrote:
>>>>>>
>>>>>> Folks,
>>>>>>
>>>>>> I'm not familiar with a thin client protocol, so, please, correct me
>>>>>> if I'm wrong - is it possible sending classes over the network for
>>>>>> different type of thin clients? It seems to me it will not work. I
>>>>>> think it is possible to use class name/or type id and we are capable
>>>>>> to instantiate both of the AffinityFunction and AffinityKeyMapper
>>> (it
>>>>>> will be required too) classes. However, this approach requires of
>>>>>> these classes to be present on a thin client side and it can be
>>> tricky
>>>>>> to configure if this client is used inside a container (e.g. with
>>>>>> spring data or something).
>>>>>>
>>>>>> Setting directly some kind of a known mapper looks more
>>>>>> straightforward and error prone approach.
>>>>>>
>>>>>> Please, correct me if I am wrong.
>>>>>>
>>>>>> On Thu, 7 Jul 2022 at 21:02, Семён Данилов <samvi...@yandex.ru>
>>> wrote:
>>>>>>>
>>>>>>> +1 to Pavel’s approach. Giving user the ability to set affinity
>>>>> function that is potentially different from the one that is on server
>>>>> doesn’t look good.
>>>>>>>
>>>>>>> I’d even go further and try sending the affinity function class
>>> itself
>>>>> over the network, so users won’t have to alter the classpath by
>>> adding the
>>>>> class that is not directly used in the codebase.
>>>>>>> On 7 Jul 2022, 21:41 +0400, Pavel Tupitsyn <ptupit...@apache.org
>>>> ,
>>>>> wrote:
>>>>>>>> Can we actually avoid any public API changes and do the
>>> following:
>>>>>>>>
>>>>>>>> When a feature flag is present and there is a custom affinity
>>>>> function on
>>>>>>>> the server, send back the class name (or binary type id, if
>>>>> possible) of
>>>>>>>> the affinity function?
>>>>>>>> Then simply instantiate it on the client and call
>>> `partition(Object
>>>>> key)`
>>>>>>>> on it?
>>>>>>>>
>>>>>>>> On Thu, Jul 7, 2022 at 8:32 PM Maxim Muzafarov <
>>> mmu...@apache.org>
>>>>> wrote:
>>>>>>>>
>>>>>>>>> Pavel,
>>>>>>>>>
>>>>>>>>> Here is my thoughts.
>>>>>>>>>
>>>>>>>>> As I mentioned before, we will need only the
>>>>>>>>> AffinityFunction#partition(Object key) method for calculation
>>> on
>>>>> the
>>>>>>>>> client side due to all the partition-to-node mappings will be
>>>>>>>>> requested asynchronously from a server node how it is
>>> happening now
>>>>>>>>> for cache groups with the default affinity function. Thus
>>> setting
>>>>> the
>>>>>>>>> AffinityFunction looks on the client side redundant and it
>>> can be
>>>>>>>>> transformed to a simple ToInFunction<Object> interface.
>>>>>>>>>
>>>>>>>>> public interface ToIntFunction<Object> {
>>>>>>>>> int applyAsInt(Object key);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> Another point that I'd like also to mention is that the cache
>>>>> already
>>>>>>>>> created in a cluster, so it will be better to eliminate the
>>>>> duplicate
>>>>>>>>> affinity function configuration (setting the number of
>>>>> partitions). It
>>>>>>>>> is fully possible to receive the number of partitions from a
>>> server
>>>>>>>>> node (the same way as it currently happening). Thus instead
>>> of the
>>>>>>>>> ToInFunction<Object> interface it will be better to use
>>>>>>>>> ToInBiFunction<Object, Integer>.
>>>>>>>>>
>>>>>>>>> public interface ToIntBiFunction<Object, Integer> {
>>>>>>>>> int applyAsInt(Object key, Integer parts);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I agree with you that "setPartitionAwarenessKeyMapper" may be
>>> is
>>>>> not
>>>>>>>>> good naming here, we can rename it and move it to the public
>>> API,
>>>>> of
>>>>>>>>> course.
>>>>>>>>>
>>>>>>>>> On Thu, 7 Jul 2022 at 20:06, Pavel Tupitsyn <
>>> ptupit...@apache.org>
>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> Maxim,
>>>>>>>>>>
>>>>>>>>>> I am confused. We were talking about a custom Affinity
>>> Function.
>>>>>>>>>> As you noted, AffinityKeyMapper is deprecated, why do we add
>>>>> something
>>>>>>>>>> named "setPartitionAwarenessKeyMapper"?
>>>>>>>>>>
>>>>>>>>>> Internal API approach is hacky.
>>>>>>>>>> IMO we should either develop a proper feature with a good
>>> public
>>>>> API, or
>>>>>>>>>> not add anything at all.
>>>>>>>>>>
>>>>>>>>>> On Thu, Jul 7, 2022 at 6:34 PM Maxim Muzafarov <
>>>>> mmu...@apache.org>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Folks,
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thank you for your comments.
>>>>>>>>>>>
>>>>>>>>>>> First of all, I'd like to point out that if the cache is
>>>>> created with
>>>>>>>>>>> a custom affinity function or the legacy AffinityKeyMapper
>>>>> interface,
>>>>>>>>>>> the thin client should be able to provide only a
>>>>> `key-to-partition`
>>>>>>>>>>> mapping function to handle the case described above. The
>>>>>>>>>>> `partition-to-node` mappings the client will receive from
>>> a
>>>>> server
>>>>>>>>>>> node it connected to. This will simplify a bit the final
>>>>> solution.
>>>>>>>>>>>
>>>>>>>>>>> ==================
>>>>>>>>>>>
>>>>>>>>>>> I've checked your suggestions and it looks like both of
>>> them
>>>>> have some
>>>>>>>>>>> sufficient drawbacks:
>>>>>>>>>>>
>>>>>>>>>>> 1. Using SystemProperty looks really hacky and we are
>>>>> explicitly
>>>>>>>>>>> complicate a thin client configuration for an end user.
>>>>>>>>>>>
>>>>>>>>>>> 2. Extending the ClientCacheConfiguration is a very good
>>> and
>>>>>>>>>>> straightforward idea, however it doesn't fit the case
>>>>> described above.
>>>>>>>>>>> Caches previously created with custom affinity
>>> functions/key
>>>>> mappers
>>>>>>>>>>> already present in the cluster, so in this case we are
>>> forcing
>>>>> a user
>>>>>>>>>>> to store an additional ClientCacheConfiguration. This is
>>> not
>>>>> good. The
>>>>>>>>>>> cache.getOrCreate(cfg) method will also be used and fire
>>> in
>>>>> turn
>>>>>>>>>>> CACHE_GET_OR_CREATE_WITH_CONFIGURATION request which is
>>> not
>>>>> necessary
>>>>>>>>>>> here. For this case using cache.name(str) is only enough.
>>>>>>>>>>>
>>>>>>>>>>> ==================
>>>>>>>>>>>
>>>>>>>>>>> I propose the following two solutions that looks very
>>>>> promising:
>>>>>>>>>>>
>>>>>>>>>>> 3. Extending cache create methdos with a
>>> ClientCacheContext in
>>>>> the
>>>>>>>>>>> IgniteClient interface. This context will contain all
>>>>> additional cache
>>>>>>>>>>> attributes like custom cache affinity mappers that map
>>> cache
>>>>> keys to
>>>>>>>>>>> partitions if a custom affinity was used on the server
>>> side
>>>>> (note that
>>>>>>>>>>> all partition-to-node mappings will be received by thin
>>> client
>>>>> from a
>>>>>>>>>>> server node).
>>>>>>>>>>>
>>>>>>>>>>> interface IgniteClientEx extends IgniteClient {
>>>>>>>>>>> ClientCache<K, V> name(String name, ClientCacheContext
>>> cctx);
>>>>>>>>>>> ClientCache<K, V> getOrCreateCache(String name,
>>>>> ClientCacheContext
>>>>>>>>>>> cctx);
>>>>>>>>>>> ClientCache<K, V>
>>> getOrCreateCache(ClientCacheConfiguration
>>>>> cfg,
>>>>>>>>>>> ClientCacheContext cctx);
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> class ClientCacheContext {
>>>>>>>>>>> setPartitionAwarenessKeyMapper(ToIntBiFunction<Object,
>>> Integer>
>>>>>>>>>>> mapper);
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> 4. Use the same approach as the IgniteCache interface
>>> does for
>>>>> the
>>>>>>>>>>> same things - adding withPartitionAwarenessKeyMapper() to
>>> the
>>>>>>>>>>> interface. This method will allow to configure the thin
>>> client
>>>>>>>>>>> execution behaviour for the partition awareness feature by
>>>>> setting a
>>>>>>>>>>> custom cache key mapper.
>>>>>>>>>>>
>>>>>>>>>>> ==================
>>>>>>>>>>>
>>>>>>>>>>> I've used the 4-th solution due to it brings much less
>>> source
>>>>> code to
>>>>>>>>>>> the Apache Ignite codebase and looks a bit simpler to
>>>>> configure for a
>>>>>>>>>>> user. I've also move the withPartitionAwarenessKeyMapper()
>>>>> method to
>>>>>>>>>>> an internal API interface which still solves a user issue
>>> with
>>>>> the
>>>>>>>>>>> partition awareness, but also specifies that the custom
>>> mapping
>>>>>>>>>>> function and deprecated AffinityKeyMapper should not be
>>> used,
>>>>> in
>>>>>>>>>>> general.
>>>>>>>>>>>
>>>>>>>>>>> Please, take a look at my patch:
>>>>>>>>>>>
>>>>>>>>>>> https://issues.apache.org/jira/browse/IGNITE-17316
>>>>>>>>>>> https://github.com/apache/ignite/pull/10140/files
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Fri, 1 Jul 2022 at 14:41, Pavel Tupitsyn <
>>>>> ptupit...@apache.org>
>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> I have no objections to extending the Thin Client
>>>>> configuration with
>>>>>>>>> a
>>>>>>>>>>>> pluggable Affinity Function.
>>>>>>>>>>>> Let's make it a normal Java setter though, system
>>> properties
>>>>> are
>>>>>>>>> hacky.
>>>>>>>>>>>> Especially when only some of the caches use custom
>>> affinity,
>>>>> as Maxim
>>>>>>>>>>>> mentioned.
>>>>>>>>>>>>
>>>>>>>>>>>> On Wed, Jun 29, 2022 at 7:20 PM Николай Ижиков <
>>>>> nizhi...@apache.org>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> +1 to have ability to specify custom affinity for PA
>>> on
>>>>> thin
>>>>>>>>> client.
>>>>>>>>>>>>>
>>>>>>>>>>>>> It seems to me custom affinity is a rare use-case of
>>>>> Ignite API.
>>>>>>>>>>>>> Propose to have SystemProperty that can specify
>>> affinity
>>>>>>>>> implementation
>>>>>>>>>>>>> for a thin client.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> 29 июня 2022 г., в 18:53, Maxim Muzafarov <
>>>>> mmu...@apache.org>
>>>>>>>>>>>>> написал(а):
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Igniters,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I've faced with a customer's cluster which has more
>>> than
>>>>> 150
>>>>>>>>> nodes
>>>>>>>>>>> and
>>>>>>>>>>>>>> most for them are the thick-client nodes. Due to
>>> each
>>>>>>>>> thick-client is
>>>>>>>>>>>>>> a full-fledged cluster topology participant it
>>> affects
>>>>> the
>>>>>>>>> cluster
>>>>>>>>>>>>>> discovery machinery during the system operation and
>>>>> adding an
>>>>>>>>>>>>>> additional overhead for using/deploying a new nodes
>>> in
>>>>> Kubernetes
>>>>>>>>>>>>>> environment. However, the main thing from my point
>>> of
>>>>> view it
>>>>>>>>>>> prevents
>>>>>>>>>>>>>> updating the client side and server side components
>>>>> independently
>>>>>>>>>>>>>> (Apache Ignite doesn't support rolling upgrade).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Accordingly to the assumptions above using thin
>>> clients
>>>>> become a
>>>>>>>>>>>>>> necessary. This looks even more attractive, since
>>> the
>>>>> thin
>>>>>>>>> client has
>>>>>>>>>>>>>> a fairly rich API over the past few releases.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The MAIN ISSUE here that blocks thin client usage is
>>>>> that for
>>>>>>>>> some of
>>>>>>>>>>>>>> cache groups a custom affinity function (and an
>>>>>>>>> AffinityKeyMapper)
>>>>>>>>>>> was
>>>>>>>>>>>>>> used which prevents enabling the Partition Awareness
>>>>> thin client
>>>>>>>>>>>>>> feature. Thus each cache request will have two hops.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Of course, we can force users to migrate to a new
>>> API,
>>>>> but this
>>>>>>>>>>>>>> becomes more difficult when Apache Ignite is part
>>> of a
>>>>> much
>>>>>>>>> larger
>>>>>>>>>>>>>> architectural solution and thus it is doent' looks
>>> so
>>>>> friendly.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The MAIN QUESTION here - does anyone know our users
>>> who
>>>>> have
>>>>>>>>>>>>>> encountered with the same issue? I want to solve
>>> such a
>>>>> problem
>>>>>>>>> once
>>>>>>>>>>>>>> and make all such users happy by implementing the
>>> general
>>>>>>>>> approach.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> = Possible solutions =
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 1. Making an affinity function pluggable (mapping
>>>>> calculations)
>>>>>>>>> on
>>>>>>>>>>> the
>>>>>>>>>>>>>> thin clients side. Currently the
>>>>> RendezvousAffinityFunction [1]
>>>>>>>>> is
>>>>>>>>>>>>>> only supported
>>>>>>>>>>>>>> for the partition awareness. A user's affinity
>>> function
>>>>> seems to
>>>>>>>>> be
>>>>>>>>>>>>>> the stateless function due to there is no machinery
>>> to
>>>>> transfer
>>>>>>>>>>> states
>>>>>>>>>>>>>> to the thin client.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Pros - a general solution for all such cases;
>>>>>>>>>>>>>> Cons - unnecessary complexity, extending public API;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 2. Creating an Ignite extension which will extend
>>> the
>>>>> thin
>>>>>>>>> client API
>>>>>>>>>>>>>> thus a user will have a full control over a
>>> destination
>>>>> node to
>>>>>>>>> which
>>>>>>>>>>>>>> requests being sent.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Pros - isolated solution, simple implementation;
>>>>>>>>>>>>>> Cons - hard to support spring-boot-thin-client etc.
>>> and
>>>>> other
>>>>>>>>>>>>>> extensions based on the thin client API;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Folks, please share your thoughts.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>
>>> https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/processors/platform/client/cache/ClientCachePartitionsRequest.java#L206
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>
>>>
>>
