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