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
>> > > > > > > > > > >
>> > > > > > > > > > >
>> > > > > > > > >
>> > > > > > >
>> > >
>>
>
