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