Unfortunately, affinity function is not rivially constructible, at least default one
вт, 12 июл. 2022 г., 10:37 Pavel Tupitsyn <ptupit...@apache.org>: > Alex, the second option is exactly what I've proposed above. > > On Tue, Jul 12, 2022 at 9:46 AM Alex Plehanov <plehanov.a...@gmail.com> > wrote: > > > Folks, why should we construct a fully functional affinity function on > the > > client side by custom client code? We need only a key to partition > mapping, > > so only this simple mapper factory will be enough. > > From my point of view there are to options possible: > > - provide ability to set partition to key mapper factory by the client > code > > or > > - serialize custom affinity functions and custom affinity key mappers to > > binary objects and send it together with ClientCachePartitionResponse > > message. In this case, explicit client configuration or some kind of > client > > code is not required, affinity function (with type id and its internals) > > will be deserialized if classes are present on thin-client side and will > be > > used implicitly (for java thin client). For other thin clients (except > > java) type id can be retrieved from the binary object and some key to > > partition mapper can be provided based on this type id (for example by > > configured factory). > > > > пн, 11 июл. 2022 г. в 16:22, Pavel Tupitsyn <ptupit...@apache.org>: > > > > > No objections to the AffinityFunctionFactory approach from my side. > > > I think it should be based on cacheName, not groupId. > > > > > > On Mon, Jul 11, 2022 at 3:52 PM Maxim Muzafarov <mmu...@apache.org> > > wrote: > > > > > > > Folks, > > > > > > > > I've done research about the proposed solution and I'd like to > discuss > > > > with you a possible options we may have. After we agreed on a > solution > > > > design I'll create a new IEP with everything we've discussed above. > > > > > > > > So, let's consider the most completed case: > > > > the RendezvousAffinityFunction with a custom affinity mapper function > > is > > > > used. > > > > > > > > > > > > ===================== > > > > > > > > The solution with sending AffintyFunction typeId. > > > > > > > > a. The deprecated AffinityKeyMapper used prior to the > AffinityFunction > > > > for calculation a key to partition mapping. > > > > See the link [1] - affFunction.partition(affinityKey(key)) > > > > > > > > b. We are able to map typeId of the RendezvousAffinityFunction to > > > > another AffinityFunction on the client side to solve the mapping > > > > problem with any combination of AffinityFunction and > > > > AffinityKeyMappers ware used. > > > > > > > > c. When the cache partitions mapping request [2] is executed we are > > > > able to send the typeId of the RendezvousAffinityFunction back to the > > > > client, however without sending the typeId's of custom affinity > > > > mappers we are not able to identify the case when a substitution of > > > > the AffinityFunction is required on the client side. > > > > > > > > For instance, > > > > > > > > cacheGroup_1 has RendezvousAffinityFunction + FirstAffinityKeyMapper > > > > cacheGroup_2 has RendezvousAffinityFunction + SecondAffinityKeyMapper > > > > > > > > Adding a deprecated classes and their corresponding typeId's to the > > > > exchange client-server protocol sound not a good idea. However, this > > > > will cover all the cases. > > > > > > > > ===================== > > > > > > > > The solution with factory method for AffintyFunction on the client > > side. > > > > > > > > We can also improve the solution that was proposed by me a few > > > > messages ago. Instead of the withPartitionAwarenessKeyMapper() [4] > > > > method which looks a bit weird we can add a factory method to the > > > > IgniteClientConfiguration that will provide a custom AffinityFunction > > > > for caches wift `non-default` mappings. > > > > > > > > The factory may looks like: > > > > > > > > class AffinityFunctionFactory implements BiFunction<Integer, Integer, > > > > AffinityFunction> { > > > > @Override public AffinityFunction apply(Integer groupId, Integer > > > > parts) { > > > > return new RendezvousAffinityFunction(false, parts); > > > > } > > > > } > > > > > > > > This cases will have a bit straightforward implementation and > explicit > > > > AffinityFunction initialization by a user. > > > > > > > > > > > > WDYT? > > > > > > > > > > > > > > > > [1] > > > > > > > > > > https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheAffinityManager.java#L185 > > > > [2] > > > > > > > > > > https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/processors/platform/client/ClientMessageParser.java#L535 > > > > [3] > > > > > > > > > > https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/cache/affinity/AffinityFunction.java#L53 > > > > [4] > > > > > > > > > > https://github.com/apache/ignite/pull/10140/files#diff-fb60155466fa2c31d6d1270c0e08f15f1ad9c1a2272fc9bb137c07d9ae5c1b98R47 > > > > > > > > On Mon, 11 Jul 2022 at 13:14, Ivan Daschinsky <ivanda...@gmail.com> > > > wrote: > > > > > > > > > > Still don't understand how type id can help me to obtain valid > mapper > > > > from > > > > > key to int32. Especially when we are talking about non-jvm > languages. > > > > > > > > > > пн, 11 июл. 2022 г., 12:33 Николай Ижиков <nizhi...@apache.org>: > > > > > > > > > > > Pavel. > > > > > > > > > > > > > It is not cryptic, it is very straightforward and builds on > > > existing > > > > > > binary type mechanism. > > > > > > > > > > > > I see the Ivan’s point here. > > > > > > As I said earlier - It must be absolutely clear for user - PA > works > > > or > > > > not. > > > > > > > > > > > > With the logic you propose it will be hiding inside Ignite > > machinery > > > > and > > > > > > logs. > > > > > > > > > > > > > Unfortunately, this is a breaking change. Currently this > scenario > > > > works > > > > > > without errors - uses default socket for custom affinity caches. > > > > > > > > > > > > It’s OK from my point of view to introduce such a change. > > > > > > > > > > > > > > > > > > > 10 июля 2022 г., в 22:05, Pavel Tupitsyn <ptupit...@apache.org > > > > > > > > написал(а): > > > > > > > > > > > > > >> providing simple function Object -> int32 in cache > configuration > > > is > > > > > > wrong > > > > > > > decision > > > > > > > > > > > > > > Because it is error-prone and does not work for all cases: > > > > > > > - You have to set it explicitly on the client for every cache. > > > > > > > - No way to do that if you get an existing cache by name. > > > > > > > > > > > > > >> still being unable to solve the problem > > > > > > > > > > > > > > I believe the proposed solution covers all use cases. > > > > > > > > > > > > > >> cryptic logic > > > > > > > > > > > > > > It is not cryptic, it is very straightforward and builds on > > > existing > > > > > > binary > > > > > > > type mechanism. > > > > > > > > > > > > > >> feature flags > > > > > > > > > > > > > > What's bad about feature flags? > > > > > > > > > > > > > >> how an ability to obtain a classname of java class can help my > > > > python or > > > > > > > C++ client map key to partition > > > > > > > > > > > > > > Not a java class name, but type id. You can set up type id > > mapping > > > > > > properly > > > > > > > and use this in any thin client. > > > > > > > This will work for .NET client, not sure about Python and C++. > > > > > > > > > > > > > > On Sun, Jul 10, 2022 at 9:33 PM Ivan Daschinsky < > > > ivanda...@gmail.com > > > > > > > > > > > wrote: > > > > > > > > > > > > > >> Guys, I simply cant understand why providing simple function > > > Object > > > > -> > > > > > > >> int32 in cache configuration is wrong decision, but > implementing > > > > cryptic > > > > > > >> logic with class names, feature flags and still being unable > to > > > > solve > > > > > > the > > > > > > >> probem is ok. I don't understand how an ability to obtain a > > > > classname of > > > > > > >> java class can help my python or C++ client map key to > > partition. > > > > > > >> > > > > > > >> Max's proposal seems to be a good variant, just change naming > a > > > > little > > > > > > bit, > > > > > > >> maybe > > > > > > >> > > > > > > >> пт, 8 июл. 2022 г., 15:35 Pavel Tupitsyn < > ptupit...@apache.org > > >: > > > > > > >> > > > > > > >>> 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 > > > > > > >>>>>>>>>>>>>>>>> > > > > > > >>>>>>>>>>>>>>>>> > > > > > > >>>>>>>>>>>>>>> > > > > > > >>>>>>>>>>>>> > > > > > > >>>>>>>>> > > > > > > >>>>>>> > > > > > > >>>>>> > > > > > > >>>> > > > > > > >>>> > > > > > > >>> > > > > > > >> > > > > > > > > > > > > > > > > > > > > > >