Hi all, Hopefully a quick question...
KT01. Will clients calculate the topic hash on the client? Based on the current state of the KIP and PR, I would have thought "no", but I ask based on the discussion around the possible use of Guava on client. Thanks, Kirk On Mon, Jan 6, 2025, at 9:11 AM, David Jacot wrote: > Hi PoAn, > > Thanks for the update. I haven't read the updated KIP yet. > > DJ02: I am not sure about using Guava as a dependency. I mentioned it more > as an inspiration/reference. I suppose that we could use it on the server > but we should definitely not use it on the client. I am not sure how others > feel about it. > > Best, > David > > On Mon, Jan 6, 2025 at 5:21 AM PoAn Yang <yangp...@gmail.com> wrote: > > > Hi Chia-Ping / David / Lucas, > > > > Happy new year and thanks for the review. > > > > DJ02: Thanks for the suggestion. I updated the PR to use Guava. > > > > DJ03: Yes, I updated the description to mention ISR change, > > add altering partition reassignment case, and mention that > > non-related topic change doesn’t trigger a rebalance. > > DJ03.1: Yes, I will keep using ModernGroup#requestMetadataRefresh > > to notify group. > > > > DJ06: Updated the PR to use Guava Hashing#combineUnordered > > function to combine topic hash. > > > > DJ07: Renamed it to MetadataHash. > > > > DJ08: Added a sample hash function to the KIP and use first byte as magic > > byte. This is also included in latest PR. > > > > DJ09: Added two paragraphs about upgraded and downgraded. > > > > DJ10: According to Lucas’s comment, I add StreamsGroupMetadataValue update > > to this KIP. > > > > Thanks, > > PoAn > > > > > > > On Dec 20, 2024, at 3:58 PM, Chia-Ping Tsai <chia7...@gmail.com> wrote: > > > > > >> because assignors are sticky. > > > > > > I forgot about that spec again :( > > > > > > > > > > > > > > > David Jacot <david.ja...@gmail.com> 於 2024年12月20日 週五 下午3:41寫道: > > > > > >> Hi Chia-Ping, > > >> > > >> DJ08: In my opinion, changing the format will be rare so it is > > >> acceptable if rebalances are triggered in this case on > > >> upgrade/downgrade. It is also what will happen if a cluster is > > >> downgraded from 4.1 (with this KIP) to 4.0. The rebalance won't change > > >> anything if the topology of the group is the same because assignors > > >> are sticky. The default ones are and we recommend custom ones to also > > >> be. > > >> > > >> Best, > > >> David > > >> > > >> On Fri, Dec 20, 2024 at 2:11 AM Chia-Ping Tsai <chia7...@apache.org> > > >> wrote: > > >>> > > >>> ummm, it does not work for downgrade as the old coordinator has no idea > > >> about new format :( > > >>> > > >>> > > >>> On 2024/12/20 00:57:27 Chia-Ping Tsai wrote: > > >>>> hi David > > >>>> > > >>>>> DJ08: > > >>>> > > >>>> That's a good question. If the "hash" lacks version control, it could > > >> trigger a series of unnecessary rebalances. However, adding additional > > >> information ("magic") to the hash does not help the upgraded coordinator > > >> determine the "version." This means that the upgraded coordinator would > > >> still trigger unnecessary rebalances because it has no way to know which > > >> format to use when comparing the hash. > > >>>> > > >>>> Perhaps we can add a new field to ConsumerGroupMetadataValue to > > >> indicate the version of the "hash." This would allow the coordinator, > > when > > >> handling subscription metadata, to compute the old hash and determine > > >> whether an epoch bump is necessary. Additionally, the coordinator can > > >> generate a new record to upgrade the hash without requiring an epoch > > bump. > > >>>> > > >>>> Another issue is whether the coordinator should cache all versions of > > >> the hash. I believe this is necessary; otherwise, during an upgrade, > > there > > >> would be extensive recomputing of old hashes. > > >>>> > > >>>> I believe this idea should also work for downgrades, and that's just > > >> my two cents. > > >>>> > > >>>> Best, > > >>>> Chia-Ping > > >>>> > > >>>> > > >>>> On 2024/12/19 14:39:41 David Jacot wrote: > > >>>>> Hi PoAn and Chia-Ping, > > >>>>> > > >>>>> Thanks for your responses. > > >>>>> > > >>>>> DJ02: Sorry, I was not clear. I was wondering whether we could > > >> compute the > > >>>>> hash without having to convert to bytes before. Guava has a nice > > >> interface > > >>>>> for this allowing to incrementally add primitive types to the hash. > > >> We can > > >>>>> discuss this in the PR as it is an implementation detail. > > >>>>> > > >>>>> DJ03: Thanks. I don't think that the replicas are updated when a > > >> broker > > >>>>> shuts down. What you said applies to the ISR. I suppose that we can > > >> rely on > > >>>>> the ISR changes to trigger updates. It is also worth noting > > >>>>> that TopicsDelta#changedTopics is updated for every change (e.g. ISR > > >>>>> change, leader change, replicas change, etc.). I suppose that it is > > >> OK but > > >>>>> it seems that it will trigger refreshes which are not necessary. > > >> However, a > > >>>>> rebalance won't be triggered because the hash won't change. > > >>>>> DJ03.1: I suppose that we will continue to rely on > > >>>>> ModernGroup#requestMetadataRefresh to notify groups that must > > >> refresh their > > >>>>> hashes. Is my understanding correct? > > >>>>> > > >>>>> DJ05: Fair enough. > > >>>>> > > >>>>> DJ06: You mention in two places that you would like to combine > > >> hashes by > > >>>>> additioning them. I wonder if this is a good practice. Intuitively, > > >> I would > > >>>>> have used XOR or hashed the hashed. Guava has a method for combining > > >>>>> hashes. It may be worth looking into the algorithm used. > > >>>>> > > >>>>> DJ07: I would rename "AllTopicHash" to "MetadataHash" in order to be > > >> more > > >>>>> generic. > > >>>>> > > >>>>> DJ08: Regarding the per topic hash, I wonder whether we should > > >> precise in > > >>>>> the KIP how we will compute it. I had the following in mind: > > >>>>> hash(topicName; numPartitions; [partitionId;sorted racks]). We could > > >> also > > >>>>> add a magic byte at the first element as a sort of version. I am not > > >> sure > > >>>>> whether it is needed though. I was thinking about this while > > >> imagining how > > >>>>> we would handle changing the format in the future. > > >>>>> > > >>>>> DJ09: It would be great if we could provide more details about > > >> backward > > >>>>> compatibility. What happens when the cluster is upgraded or > > >> downgraded? > > >>>>> > > >>>>> DJ10: We should update KIP-1071. It may be worth pigging them in the > > >>>>> discussion thread of KIP-1071. > > >>>>> > > >>>>> Best, > > >>>>> David > > >>>>> > > >>>>> On Tue, Dec 17, 2024 at 9:25 AM PoAn Yang <yangp...@gmail.com> > > >> wrote: > > >>>>> > > >>>>>> Hi Chia-Ping / David / Andrew, > > >>>>>> > > >>>>>> Thanks for the review and suggestions. > > >>>>>> > > >>>>>> DJ01: Removed all implementation details. > > >>>>>> > > >>>>>> DJ02: Does the “incrementally” mean that we only calculate the > > >> difference > > >>>>>> parts? > > >>>>>> For example, if the number of partition change, we only calculate > > >> the hash > > >>>>>> of number of partition and reconstruct it to the topic hash. > > >>>>>> IMO, we only calculate topic hash one time. With cache mechanism, > > >> the > > >>>>>> value can be reused in different groups on a same broker. > > >>>>>> The CPU usage for this part is not very high. > > >>>>>> > > >>>>>> DJ03: Added the update path to KIP for both cases. > > >>>>>> > > >>>>>> DJ04: Yes, it’s a good idea. With cache mechanism and single hash > > >> per > > >>>>>> group, we can balance cpu and disk usage. > > >>>>>> > > >>>>>> DJ05: Currently, the topic hash is only used in coordinator. > > >> However, the > > >>>>>> metadata image is used in many different places. > > >>>>>> How about we move the hash to metadata image when we find more use > > >> cases? > > >>>>>> > > >>>>>> AS1, AS2: Thanks for the reminder. I will simply delete > > >>>>>> ShareGroupPartitionMetadataKey/Value and add a new field to > > >>>>>> ShareGroupMetadataValue. > > >>>>>> > > >>>>>> Best, > > >>>>>> PoAn > > >>>>>> > > >>>>>>> On Dec 17, 2024, at 5:50 AM, Andrew Schofield < > > >>>>>> andrew_schofield_j...@outlook.com> wrote: > > >>>>>>> > > >>>>>>> Hi PoAn, > > >>>>>>> Thanks for the KIP. > > >>>>>>> > > >>>>>>> AS1: From the point of view of share groups, the API and record > > >> schema > > >>>>>>> definitions are unstable in AK 4.0. In AK 4.1, we will start > > >> supporting > > >>>>>> proper > > >>>>>>> versioning. As a result, I think you do not need to deprecate > > >> the fields > > >>>>>> in the > > >>>>>>> ShareGroupPartitionMetadataValue. Just include the schema for > > >> the fields > > >>>>>>> which are actually needed, and I'll update the schema in the > > >> code when > > >>>>>>> the KIP is implemented. > > >>>>>>> > > >>>>>>> AS2: In the event that DJ04 actually removes the need for > > >>>>>>> ConsumerGroupPartitionMetadataKey/Value entirely, I would simply > > >>>>>>> delete ShareGroupPartitionMetadataKey/Value, assuming that it is > > >>>>>>> accepted in time for AK 4.1. > > >>>>>>> > > >>>>>>> > > >>>>>>> Thanks, > > >>>>>>> Andrew > > >>>>>>> ________________________________________ > > >>>>>>> From: Chia-Ping Tsai <chia7...@apache.org> > > >>>>>>> Sent: 16 December 2024 16:27 > > >>>>>>> To: dev@kafka.apache.org <dev@kafka.apache.org> > > >>>>>>> Subject: Re: [DISCUSS] KIP-1101: Trigger rebalance on rack > > >> topology > > >>>>>> changes > > >>>>>>> > > >>>>>>> hi David > > >>>>>>> > > >>>>>>>> DJ05 > > >>>>>>> > > >>>>>>> One of the benefits of having a single hash per group (DJ04) is > > >> the > > >>>>>> reduction in the size of stored data. Additionally, the cost of > > >>>>>> re-computing can be minimized thanks to caching. So I'm + 1 to > > >> DJ04. > > >>>>>> However, the advantage of storing the topic cache in the metadata > > >> image is > > >>>>>> somewhat unclear to me. Could you please provide more details on > > >> what you > > >>>>>> mean by "tight"?Furthermore, since the metadata image is a > > >> thread-safe > > >>>>>> object, we need to ensure that the lazy initialization is also > > >> thread-safe. > > >>>>>> If no other components require the cache, it would be better to > > >> keep the > > >>>>>> caches within the coordinator. > > >>>>>>> > > >>>>>>> Best, > > >>>>>>> Chia-Ping > > >>>>>>> > > >>>>>>> On 2024/12/16 14:01:35 David Jacot wrote: > > >>>>>>>> Hi PoAn, > > >>>>>>>> > > >>>>>>>> Thanks for the KIP. I have some comments about it. > > >>>>>>>> > > >>>>>>>> DJ01: Please, remove all the code from the KIP. We only care > > >> about > > >>>>>> public > > >>>>>>>> interface changes, not about implementation details. > > >>>>>>>> DJ02: Regarding the hash computation, I agree that we should use > > >>>>>> Murmur3. > > >>>>>>>> However, I don't quite like the implementation that you shared. > > >> I > > >>>>>> wonder if > > >>>>>>>> we could make it work incrementally instead of computing a hash > > >> of > > >>>>>>>> everything and combining them. > > >>>>>>>> DJ03: Regarding the cache, my understanding is that the cache is > > >>>>>> populated > > >>>>>>>> when a topic without hash is seen in a HB request and the cache > > >> is > > >>>>>> cleaned > > >>>>>>>> up when topics are deleted based on the metadata image. > > >> However, the > > >>>>>> update > > >>>>>>>> path is not clear. Let's say that a partition is added to a > > >> topic, how > > >>>>>> does > > >>>>>>>> it detect it? Let's also imagine that the racks of a partition > > >> have > > >>>>>>>> changed, how does it detect it? In the KIP, it would be nice to > > >> be clear > > >>>>>>>> about those. > > >>>>>>>> DJ04: I wonder whether we should go with a single hash per > > >> group. Your > > >>>>>>>> argument against it is that it would require to re-compute the > > >> hash of > > >>>>>> all > > >>>>>>>> the topics when it needs to be computed. In my opinion, we could > > >>>>>> leverage > > >>>>>>>> the cached hash per topic to compute the hash of all the > > >> subscribed > > >>>>>> ones. > > >>>>>>>> We could basically combine all the hashes without having to > > >> compute all > > >>>>>> of > > >>>>>>>> them. This approach has a few benefits. 1) We could get rid of > > >>>>>>>> the ConsumerGroupPartitionMetadata record as we could store the > > >> hash > > >>>>>> with > > >>>>>>>> the group epoch. 2) We could get rid of the Map that we keep in > > >> each > > >>>>>> group > > >>>>>>>> to store the hashed corresponding to the subscribed topics. > > >>>>>>>> DJ05: Regarding the cache again, I wonder if we should actually > > >> store > > >>>>>> the > > >>>>>>>> hash in the metadata image instead of maintaining it somewhere > > >> else. We > > >>>>>>>> could still lazily compute it. The benefit is that the value > > >> would be > > >>>>>> tight > > >>>>>>>> to the topic. I have not really looked into it. Would it be an > > >> option? > > >>>>>>>> > > >>>>>>>> I'll be away for two weeks starting from Saturday. I kindly ask > > >> you to > > >>>>>> wait > > >>>>>>>> on me if we cannot conclude this week. > > >>>>>>>> > > >>>>>>>> Best, > > >>>>>>>> David > > >>>>>>>> > > >>>>>>>> On Tue, Nov 5, 2024 at 1:43 PM Frank Yang <yangp...@gmail.com> > > >> wrote: > > >>>>>>>> > > >>>>>>>>> Hi Chia-Ping, > > >>>>>>>>> > > >>>>>>>>> Thanks for the review and suggestions. > > >>>>>>>>> > > >>>>>>>>> Q0: Add how rack change and how it affects topic partition. > > >>>>>>>>> > > >>>>>>>>> Q1: Add why we need a balance algorithm to Motivation section. > > >>>>>>>>> > > >>>>>>>>> Q2: After checking again, we don’t need to update cache when > > >> we replay > > >>>>>>>>> records. We only need to renew it in consumer heartbeat. > > >>>>>>>>> > > >>>>>>>>> Q3: Add a new section “Topic Hash Function”. > > >>>>>>>>> > > >>>>>>>>> Thanks. > > >>>>>>>>> PoAn > > >>>>>>>>> > > >>>>>>>>>> On Nov 1, 2024, at 4:39 PM, Chia-Ping Tsai < > > >> chia7...@gmail.com> > > >>>>>> wrote: > > >>>>>>>>>> > > >>>>>>>>>> hi PoAn > > >>>>>>>>>> > > >>>>>>>>>> Thanks for for this KIP! > > >>>>>>>>>> > > >>>>>>>>>> Q0: Could you add more details about `A topic partition has > > >> rack > > >>>>>> change`? > > >>>>>>>>>> IIRC, the "rack change" includes both follower and leader, > > >> right? > > >>>>>>>>>> > > >>>>>>>>>> Q1: Could you please add the 'concerns' we discussed to the > > >> Motivation > > >>>>>>>>>> section? This should include topics like 'computations' and > > >> 'space > > >>>>>>>>> usage'. > > >>>>>>>>>> > > >>>>>>>>>> Q2: `The group coordinator can leverage it to add a new topic > > >>>>>> hash.`This > > >>>>>>>>>> description seems a bit off to me. Why do we need to update > > >> the cache > > >>>>>> at > > >>>>>>>>>> this phase? The cache is intended to prevent duplicate > > >> computations > > >>>>>>>>> caused > > >>>>>>>>>> by heartbeat requests that occur between two metadata change > > >> events. > > >>>>>>>>>> Therefore, we could even remove the changed topics from > > >> caches on a > > >>>>>>>>>> metadata change, as the first heartbeat request would update > > >> the > > >>>>>> caches > > >>>>>>>>> for > > >>>>>>>>>> all changed topics. > > >>>>>>>>>> > > >>>>>>>>>> Q3: Could you please include a section about the choice of > > >> hash > > >>>>>>>>>> implementation? The hash implementation must be consistent > > >> across > > >>>>>>>>> different > > >>>>>>>>>> JDKs, so we use Murmur3 to generate the hash value. > > >>>>>>>>>> > > >>>>>>>>>> Best, > > >>>>>>>>>> Chia-Ping > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> Frank Yang <yangp...@gmail.com> 於 2024年11月1日 週五 下午3:57寫道: > > >>>>>>>>>> > > >>>>>>>>>>> Hi all, > > >>>>>>>>>>> > > >>>>>>>>>>> I would like to start a discussion thread on KIP-1101. > > >> Trigger > > >>>>>> rebalance > > >>>>>>>>>>> on rack topology changes. In this KIP, we aim to use less > > >> memory / > > >>>>>> disk > > >>>>>>>>>>> resources to detect rack changes in the new coordinator. > > >>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>> > > >>>>>> > > >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1101%3A+Trigger+rebalance+on+rack+topology+changes > > >>>>>>>>>>> > > >>>>>>>>>>> Please take a look and feel free to share any thoughts. > > >>>>>>>>>>> > > >>>>>>>>>>> Thanks. > > >>>>>>>>>>> PoAn > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>> > > >>>>>> > > >>>>>> > > >>>>> > > >>>> > > >> > > > > >