Nikolay,
> Add ability to sent custom affinity class name.
Can you please elaborate? What is sent where?
> If `partitionAwarenessEnabled=true` but custom affinity can’t be
instantiated on the client - throw an exception
Unfortunately, this is a breaking change. Currently this scenario works
without errors - uses default socket for custom affinity caches.
On Fri, Jul 8, 2022 at 3:23 PM Николай Ижиков <nizhi...@apache.org> wrote:
> 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
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>
> >>>
> >>
>
>
