Hi Christo, Thanks for the explanation. I think it would be good if you could add that into the KIP.
Otherwise, LGTM. Thank you. Luke On Mon, May 13, 2024 at 11:55 PM Christo Lolov <christolo...@gmail.com> wrote: > Heya! > > re Kamal - Okay, I believe I understand what you mean and I agree. I have > made the following change > > ``` > > During tiered storage disablement, when RemoteLogManager#stopPartition() is > called: > > - Tasks scheduled for the topic-partitions in the > RemoteStorageCopierThreadPool will be canceled. > - If the disablement policy is retain, scheduled tasks for the > topic-partitions in the RemoteDataExpirationThreadPool will remain > unchanged. > - If the disablement policy is delete, we will first advance the log > start offset and we will let tasks scheduled for the topic-partitions in > the RemoteDataExpirationThreadPool to successfully delete all remote > segments before the log start offset and then unregister themselves. > > ``` > > re Luke - I checked once again. As far as I understand when a broker goes > down all replicas it hosts go to OfflineReplica state in the state machine > the controller maintains. The moment the broker comes back up again the > state machine resends StopReplica based on > ``` > > * OfflineReplica -> ReplicaDeletionStarted > * --send StopReplicaRequest to the replica (with deletion) > > ``` > from ReplicaStateMachine.scala. So I was wrong and you are right, we do not > appear to be sending constant requests today. I believe it is safe for us > to follow a similar approach i.e. if a replica comes online again we resend > the StopReplica. > > If you don't notice any more problems I will aim to start a VOTE tomorrow > so we can get at least part of this KIP in 3.8. > > Best, > Christo > > On Fri, 10 May 2024 at 11:11, Luke Chen <show...@gmail.com> wrote: > > > Hi Christo, > > > > > 1. I am not certain I follow the question. From DISABLED you can only > go > > to > > ENABLED regardless of whether your cluster is backed by Zookeeper or > KRaft. > > Am I misunderstanding your point? > > > > Yes, you're right. > > > > > 4. I was thinking that if there is a mismatch we will just fail > accepting > > the request for disablement. This should be the same in both Zookeeper > and > > KRaft. Or am I misunderstanding your question? > > > > OK, sounds good. > > > > > 6. I think my current train of thought is that there will be unlimited > > retries until all brokers respond in a similar way to how deletion of a > > topic works today in ZK. In the meantime the state will continue to be > > DISABLING. Do you have a better suggestion? > > > > I don't think infinite retries is a good idea since if a broker is down > > forever, this request will never complete. > > You mentioned the existing topic deletion is using the similar pattern, > how > > does it handle this issue? > > > > Thanks. > > Luke > > > > On Thu, May 9, 2024 at 9:21 PM Christo Lolov <christolo...@gmail.com> > > wrote: > > > > > Heya! > > > > > > re: Luke > > > > > > 1. I am not certain I follow the question. From DISABLED you can only > go > > to > > > ENABLED regardless of whether your cluster is backed by Zookeeper or > > KRaft. > > > Am I misunderstanding your point? > > > > > > 2. Apologies, this was a leftover from previous versions. I have > updated > > > the Zookeeper section. The steps ought to be: controller receives > change, > > > commits necessary data to Zookeeper, enqueues disablement and starts > > > sending StopReplicas request to brokers; brokers receive StopReplicas > and > > > propagate them all the way to RemoteLogManager#stopPartitions which > takes > > > care of the rest. > > > > > > 3. Correct, it should say DISABLED - this should now be corrected. > > > > > > 4. I was thinking that if there is a mismatch we will just fail > accepting > > > the request for disablement. This should be the same in both Zookeeper > > and > > > KRaft. Or am I misunderstanding your question? > > > > > > 5. Yeah. I am now doing a second pass on all diagrams and will update > > them > > > by the end of the day! > > > > > > 6. I think my current train of thought is that there will be unlimited > > > retries until all brokers respond in a similar way to how deletion of a > > > topic works today in ZK. In the meantime the state will continue to be > > > DISABLING. Do you have a better suggestion? > > > > > > re: Kamal > > > > > > Yep, I will update all diagrams > > > > > > I am not certain I follow the reasoning for making retain and delete > the > > > same. Deletion when the policy is retain happens asynchronously due to > > > expiration. I think that deletion when the policy is delete ought to > (at > > > least for the initial implementation) happen synchronously. Should > people > > > run into timeout problems we can always then have a follow-up KIP where > > we > > > make it asynchronous. > > > > > > Best, > > > Christo > > > > > > On Tue, 7 May 2024 at 10:04, Kamal Chandraprakash < > > > kamal.chandraprak...@gmail.com> wrote: > > > > > > > Hi Christo, > > > > > > > > Thanks for the update! > > > > > > > > For both the policies "retain" and "delete", can we maintain the same > > > > approach to delete the segments async? > > > > > > > > > If the disablement policy is set to delete, the Log start offset > > (LSO) > > > is > > > > updated to match the Local Log Start Offset and the remote log is > > deleted > > > > by calling the RemoteStorageManager#deleteLogSegmentData(). > > > > > > > > In the KIP, it's mentioned that when the disable policy is set to > > > "delete", > > > > the remote-log-segments will be > > > > deleted in-sync. The stopPartition call might get timed out when the > > > number > > > > of remote log segments to > > > > delete is huge. We can further extend the same approach for the topic > > > > deletion requests. > > > > > > > > Also, Could you please update the state diagram about the > transitions? > > It > > > > is not clear when to transit from > > > > DISABLING to DISABLED state? > > > > > > > > -- > > > > Kamal > > > > > > > > On Mon, May 6, 2024 at 6:55 PM Kamal Chandraprakash < > > > > kamal.chandraprak...@gmail.com> wrote: > > > > > > > > > Ignore the above message. Got the answers after reading the state > > > > > transition section. > > > > > > > > > > > If the disablement policy is delete, tasks scheduled for the > > > > > topic-partitions in the RemoteDataExpirationThreadPool will also be > > > > > canceled. > > > > > > > > > > We are deleting the segments synchronously. Should we delete them > > > > > asynchronously? The same approach can be extended to topic deletion > > > > > requests. > > > > > > > > > > > 6. In ZK mode, what will the controller do if the "stopReplicas" > > > > > responses not received from all brokers? Reverting the changes? > > > > > > > > > > Since we are deleting the segments synchronously. This case can be > > > bound > > > > > to happen when the number of remote log segments to > > > > > delete is huge. > > > > > > > > > > > > > > > On Mon, May 6, 2024, 18:12 Kamal Chandraprakash < > > > > > kamal.chandraprak...@gmail.com> wrote: > > > > > > > > > >> Hi Christo, > > > > >> > > > > >> Thanks for the update! > > > > >> > > > > >> 1. In the ZK mode, how will the transition from DISABLING to > > DISABLED > > > > >> state happen? > > > > >> For the "retain" policy, until we delete all the remote-log > > segments, > > > > the > > > > >> state will be > > > > >> DISABLING and the deletion can happen only when they breach either > > the > > > > >> retention > > > > >> time (or) size. > > > > >> > > > > >> How does the controller monitor that all the remote log segments > are > > > > >> deleted for all > > > > >> the partitions of the topic before transitioning the state to > > > DISABLED? > > > > >> > > > > >> 2. In Kraft, we have only ENABLED -> DISABLED state. How are we > > > > >> supporting the case > > > > >> "retain" -> "enable"? > > > > >> > > > > >> If the remote storage is degraded, we want to avoid uploading the > > > > >> segments temporarily > > > > >> and resume back once the remote storage is healthy. Is the case > > > > supported? > > > > >> > > > > >> > > > > >> > > > > >> On Fri, May 3, 2024 at 12:12 PM Luke Chen <show...@gmail.com> > > wrote: > > > > >> > > > > >>> Also, I think using `stopReplicas` request is a good idea because > > it > > > > >>> won't cause any problems while migrating to KRaft mode. > > > > >>> The stopReplicas request is one of the request that KRaft > > controller > > > > >>> will send to ZK brokers during migration. > > > > >>> > > > > >>> Thanks. > > > > >>> Luke > > > > >>> > > > > >>> On Fri, May 3, 2024 at 11:48 AM Luke Chen <show...@gmail.com> > > wrote: > > > > >>> > > > > >>>> Hi Christo, > > > > >>>> > > > > >>>> Thanks for the update. > > > > >>>> > > > > >>>> Questions: > > > > >>>> 1. For this > > > > >>>> "The possible state transition from DISABLED state is to the > > > ENABLED." > > > > >>>> I think it only applies for KRaft mode. In ZK mode, the possible > > > state > > > > >>>> is "DISABLING", right? > > > > >>>> > > > > >>>> 2. For this: > > > > >>>> "If the cluster is using Zookeeper as the control plane, > enabling > > > > >>>> remote storage for a topic triggers the controller to send this > > > > information > > > > >>>> to Zookeeper. Each broker listens for changes in Zookeeper, and > > > when a > > > > >>>> change is detected, the broker triggers > > > > >>>> RemoteLogManager#onLeadershipChange()." > > > > >>>> > > > > >>>> I think the way ZK brokers knows the leadership change is by > > getting > > > > >>>> the LeaderAndISRRequeset from the controller, not listening for > > > > changes in > > > > >>>> ZK. > > > > >>>> > > > > >>>> 3. In the KRaft handler steps, you said: > > > > >>>> "The controller also updates the Topic metadata to increment the > > > > >>>> tiered_epoch and update the tiered_stateto DISABLING state." > > > > >>>> > > > > >>>> Should it be "DISABLED" state since it's KRaft mode? > > > > >>>> > > > > >>>> 4. I was thinking how we handle the tiered_epoch not match > error. > > > > >>>> For ZK, I think the controller won't write any data into ZK > Znode, > > > > >>>> For KRaft, either configRecord or updateTopicMetadata records > > won't > > > be > > > > >>>> written. > > > > >>>> Is that right? Because the current workflow makes me think there > > > will > > > > >>>> be partial data updated in ZK/KRaft when tiered_epoch error. > > > > >>>> > > > > >>>> 5. Since we changed to use stopReplicas (V5) request now, the > > > diagram > > > > >>>> for ZK workflow might also need to update. > > > > >>>> > > > > >>>> 6. In ZK mode, what will the controller do if the "stopReplicas" > > > > >>>> responses not received from all brokers? Reverting the changes? > > > > >>>> This won't happen in KRaft mode because it's broker's > > responsibility > > > > to > > > > >>>> fetch metadata update from controller. > > > > >>>> > > > > >>>> > > > > >>>> Thank you. > > > > >>>> Luke > > > > >>>> > > > > >>>> > > > > >>>> On Fri, Apr 19, 2024 at 10:23 PM Christo Lolov < > > > > christolo...@gmail.com> > > > > >>>> wrote: > > > > >>>> > > > > >>>>> Heya all! > > > > >>>>> > > > > >>>>> I have updated KIP-950. A list of what I have updated is: > > > > >>>>> > > > > >>>>> * Explicitly state that Zookeeper-backed clusters will have > > ENABLED > > > > -> > > > > >>>>> DISABLING -> DISABLED while KRaft-backed clusters will only > have > > > > ENABLED -> > > > > >>>>> DISABLED > > > > >>>>> * Added two configurations for the new thread pools and > explained > > > > >>>>> where values will be picked-up mid Kafka version upgrade > > > > >>>>> * Explained how leftover remote partitions will be scheduled > for > > > > >>>>> deletion > > > > >>>>> * Updated the API to use StopReplica V5 rather than a whole new > > > > >>>>> controller-to-broker API > > > > >>>>> * Explained that the disablement procedure will be triggered by > > the > > > > >>>>> controller listening for an (Incremental)AlterConfig change > > > > >>>>> * Explained that we will first move log start offset and then > > > issue a > > > > >>>>> deletion > > > > >>>>> * Went into more details that changing > remote.log.disable.policy > > > > after > > > > >>>>> disablement won't do anything and that if a customer would like > > > > additional > > > > >>>>> data deleted they would have to use already existing methods > > > > >>>>> > > > > >>>>> Let me know if there are any new comments or I have missed > > > something! > > > > >>>>> > > > > >>>>> Best, > > > > >>>>> Christo > > > > >>>>> > > > > >>>>> On Mon, 15 Apr 2024 at 12:40, Christo Lolov < > > > christolo...@gmail.com> > > > > >>>>> wrote: > > > > >>>>> > > > > >>>>>> Heya Doguscan, > > > > >>>>>> > > > > >>>>>> I believe that the state of the world after this KIP will be > the > > > > >>>>>> following: > > > > >>>>>> > > > > >>>>>> For Zookeeper-backed clusters there will be 3 states: ENABLED, > > > > >>>>>> DISABLING and DISABLED. We want this because Zookeeper-backed > > > > clusters will > > > > >>>>>> await a confirmation from the brokers that they have indeed > > > stopped > > > > >>>>>> tiered-related operations on the topic. > > > > >>>>>> > > > > >>>>>> For KRaft-backed clusters there will be only 2 states: ENABLED > > and > > > > >>>>>> DISABLED. KRaft takes a fire-and-forget approach for topic > > > > deletion. I > > > > >>>>>> believe the same approach ought to be taken for tiered topics. > > The > > > > >>>>>> mechanism which will ensure that leftover state in remote due > to > > > > failures > > > > >>>>>> is cleaned up to me is the retention mechanism. In today's > > code, a > > > > leader > > > > >>>>>> deletes all segments it finds in remote with offsets below the > > log > > > > start > > > > >>>>>> offset. I believe this will be good enough for cleaning up > > > leftover > > > > state > > > > >>>>>> in remote due to failures. > > > > >>>>>> > > > > >>>>>> I know that quite a few changes have been discussed so I will > > aim > > > to > > > > >>>>>> put them on paper in the upcoming days and let everyone know! > > > > >>>>>> > > > > >>>>>> Best, > > > > >>>>>> Christo > > > > >>>>>> > > > > >>>>>> On Tue, 9 Apr 2024 at 14:49, Doğuşcan Namal < > > > > namal.dogus...@gmail.com> > > > > >>>>>> wrote: > > > > >>>>>> > > > > >>>>>>> +1 let's not introduce a new api and mark it immediately as > > > > >>>>>>> deprecated :) > > > > >>>>>>> > > > > >>>>>>> On your second comment Luke, one thing we need to clarify is > > when > > > > do > > > > >>>>>>> we consider remote storage to be DISABLED for a topic? > > > > >>>>>>> Particularly, what is the state when the remote storage is > > being > > > > >>>>>>> deleted in case of disablement.policy=delete? Is it DISABLING > > or > > > > DISABLED? > > > > >>>>>>> > > > > >>>>>>> If we move directly to the DISABLED state, > > > > >>>>>>> > > > > >>>>>>> a) in case of failures, the leaders should continue remote > > > storage > > > > >>>>>>> deletion even if the topic is moved to the DISABLED state, > > > > otherwise we > > > > >>>>>>> risk having stray data on remote storage. > > > > >>>>>>> b) on each restart, we should initiate the remote storage > > > deletion > > > > >>>>>>> because although we replayed a record with a DISABLED state, > we > > > > can not be > > > > >>>>>>> sure if the remote data is deleted or not. > > > > >>>>>>> > > > > >>>>>>> We could either consider keeping the remote topic in > DISABLING > > > > state > > > > >>>>>>> until all of the remote storage data is deleted, or we need > an > > > > additional > > > > >>>>>>> mechanism to handle the remote stray data. > > > > >>>>>>> > > > > >>>>>>> The existing topic deletion, for instance, handles stray logs > > on > > > > >>>>>>> disk by detecting them on KafkaBroker startup and deleting > > before > > > > the > > > > >>>>>>> ReplicaManager is started. > > > > >>>>>>> Maybe we need a similar mechanism here as well if we don't > > want a > > > > >>>>>>> DISABLING state. Otherwise, we need a callback from Brokers > to > > > > validate > > > > >>>>>>> that remote storage data is deleted and now we could move to > > the > > > > DISABLED > > > > >>>>>>> state. > > > > >>>>>>> > > > > >>>>>>> Thanks. > > > > >>>>>>> > > > > >>>>>>> On Tue, 9 Apr 2024 at 12:45, Luke Chen <show...@gmail.com> > > > wrote: > > > > >>>>>>> > > > > >>>>>>>> Hi Christo, > > > > >>>>>>>> > > > > >>>>>>>> > I would then opt for moving information from > > > DisableRemoteTopic > > > > >>>>>>>> within the StopReplicas API which will then disappear in > KRaft > > > > >>>>>>>> world as it > > > > >>>>>>>> is already scheduled for deprecation. What do you think? > > > > >>>>>>>> > > > > >>>>>>>> Sounds good to me. > > > > >>>>>>>> > > > > >>>>>>>> Thanks. > > > > >>>>>>>> Luke > > > > >>>>>>>> > > > > >>>>>>>> On Tue, Apr 9, 2024 at 6:46 PM Christo Lolov < > > > > >>>>>>>> christolo...@gmail.com> wrote: > > > > >>>>>>>> > > > > >>>>>>>> > Heya Luke! > > > > >>>>>>>> > > > > > >>>>>>>> > I thought a bit more about it and I reached the same > > > conclusion > > > > >>>>>>>> as you for > > > > >>>>>>>> > 2 as a follow-up from 1. In other words, in KRaft world I > > > don't > > > > >>>>>>>> think the > > > > >>>>>>>> > controller needs to wait for acknowledgements for the > > brokers. > > > > >>>>>>>> All we care > > > > >>>>>>>> > about is that the leader (who is responsible for > > > > >>>>>>>> archiving/deleting data in > > > > >>>>>>>> > tiered storage) knows about the change and applies it > > > properly. > > > > >>>>>>>> If there is > > > > >>>>>>>> > a leadership change halfway through the operation then the > > new > > > > >>>>>>>> leader still > > > > >>>>>>>> > needs to apply the message from the state topic and we > know > > > > that a > > > > >>>>>>>> > disable-message will be applied before a > > > reenablement-message. I > > > > >>>>>>>> will > > > > >>>>>>>> > change the KIP later today/tomorrow morning to reflect > this > > > > >>>>>>>> reasoning. > > > > >>>>>>>> > > > > > >>>>>>>> > However, with this I believe that introducing a new API > just > > > for > > > > >>>>>>>> > Zookeeper-based clusters (i.e. DisableRemoteTopic) > becomes a > > > bit > > > > >>>>>>>> of an > > > > >>>>>>>> > overkill. I would then opt for moving information from > > > > >>>>>>>> DisableRemoteTopic > > > > >>>>>>>> > within the StopReplicas API which will then disappear in > > KRaft > > > > >>>>>>>> world as it > > > > >>>>>>>> > is already scheduled for deprecation. What do you think? > > > > >>>>>>>> > > > > > >>>>>>>> > Best, > > > > >>>>>>>> > Christo > > > > >>>>>>>> > > > > > >>>>>>>> > On Wed, 3 Apr 2024 at 07:59, Luke Chen <show...@gmail.com > > > > > > wrote: > > > > >>>>>>>> > > > > > >>>>>>>> > > Hi Christo, > > > > >>>>>>>> > > > > > > >>>>>>>> > > 1. I agree with Doguscan that in KRaft mode, the > > controller > > > > >>>>>>>> won't send > > > > >>>>>>>> > RPCs > > > > >>>>>>>> > > to the brokers (except in the migration path). > > > > >>>>>>>> > > So, I think we could adopt the similar way we did to > > > > >>>>>>>> > `AlterReplicaLogDirs` > > > > >>>>>>>> > > ( > > > > >>>>>>>> > > KIP-858 > > > > >>>>>>>> > > < > > > > >>>>>>>> > > > > > > >>>>>>>> > > > > > >>>>>>>> > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-858%3A+Handle+JBOD+broker+disk+failure+in+KRaft#KIP858:HandleJBODbrokerdiskfailureinKRaft-Intra-brokerreplicamovement > > > > >>>>>>>> > > >) > > > > >>>>>>>> > > that let the broker notify controller any update, > instead > > of > > > > >>>>>>>> controller > > > > >>>>>>>> > to > > > > >>>>>>>> > > broker. And once the controller receives all the > complete > > > > >>>>>>>> requests from > > > > >>>>>>>> > > brokers, it'll enter "Disabled" state. WDYT? > > > > >>>>>>>> > > > > > > >>>>>>>> > > 2. Why should we wait until all brokers to respond > before > > > > >>>>>>>> moving to > > > > >>>>>>>> > > "Disabled" state in "KRaft mode"? > > > > >>>>>>>> > > Currently, only the leader node does the remote log > > > > >>>>>>>> upload/fetch tasks, > > > > >>>>>>>> > so > > > > >>>>>>>> > > does that mean the controller only need to make sure the > > > > leader > > > > >>>>>>>> completes > > > > >>>>>>>> > > the stopPartition? > > > > >>>>>>>> > > If during the leader node stopPartition process > triggered > > > > >>>>>>>> leadership > > > > >>>>>>>> > > change, then the new leader should receive and apply the > > > > >>>>>>>> configRecord > > > > >>>>>>>> > > update before the leadership change record based on the > > > KRaft > > > > >>>>>>>> design, > > > > >>>>>>>> > which > > > > >>>>>>>> > > means there will be no gap that the follower node > becomes > > > the > > > > >>>>>>>> leader and > > > > >>>>>>>> > > starting doing unexpected upload/fetch tasks, right? > > > > >>>>>>>> > > I agree we should make sure in ZK mode, all brokers are > > > > >>>>>>>> completed the > > > > >>>>>>>> > > stopPartitions before moving to "Disabled" state because > > ZK > > > > >>>>>>>> node watcher > > > > >>>>>>>> > is > > > > >>>>>>>> > > working in a separate thread. But not sure about KRaft > > mode. > > > > >>>>>>>> > > > > > > >>>>>>>> > > Thanks. > > > > >>>>>>>> > > Luke > > > > >>>>>>>> > > > > > > >>>>>>>> > > > > > > >>>>>>>> > > On Fri, Mar 29, 2024 at 4:14 PM Christo Lolov < > > > > >>>>>>>> christolo...@gmail.com> > > > > >>>>>>>> > > wrote: > > > > >>>>>>>> > > > > > > >>>>>>>> > > > Heya everyone! > > > > >>>>>>>> > > > > > > > >>>>>>>> > > > re: Doguscan > > > > >>>>>>>> > > > > > > > >>>>>>>> > > > I believe the answer to 101 needs a bit more > discussion. > > > As > > > > >>>>>>>> far as I > > > > >>>>>>>> > > know, > > > > >>>>>>>> > > > tiered storage today has methods to update a metadata > > of a > > > > >>>>>>>> segment to > > > > >>>>>>>> > say > > > > >>>>>>>> > > > "hey, I would like this deleted", but actual deletion > is > > > > left > > > > >>>>>>>> to plugin > > > > >>>>>>>> > > > implementations (or any background cleaners). In other > > > > words, > > > > >>>>>>>> there is > > > > >>>>>>>> > no > > > > >>>>>>>> > > > "immediate" deletion. In this KIP, we would like to > > > continue > > > > >>>>>>>> doing the > > > > >>>>>>>> > > same > > > > >>>>>>>> > > > if the retention policy is set to delete. So I believe > > the > > > > >>>>>>>> answer is > > > > >>>>>>>> > > > actually that a) we will update the metadata of the > > > segments > > > > >>>>>>>> to mark > > > > >>>>>>>> > them > > > > >>>>>>>> > > > as deleted and b) we will advance the log start > offset. > > > Any > > > > >>>>>>>> deletion of > > > > >>>>>>>> > > > actual files will still be delegated to plugin > > > > >>>>>>>> implementations. I > > > > >>>>>>>> > believe > > > > >>>>>>>> > > > this is further supported by > > > > >>>>>>>> "*remote.log.disable.policy=delete:* Logs > > > > >>>>>>>> > > that > > > > >>>>>>>> > > > are archived in the remote storage will not be part of > > the > > > > >>>>>>>> contiguous > > > > >>>>>>>> > > > "active" log and will be deleted asynchronously as > part > > of > > > > the > > > > >>>>>>>> > > disablement > > > > >>>>>>>> > > > process" > > > > >>>>>>>> > > > > > > > >>>>>>>> > > > Following from the above, I believe for 102 it is fine > > to > > > > >>>>>>>> allow setting > > > > >>>>>>>> > > of > > > > >>>>>>>> > > > remote.log.disable.policy on a disabled topic in much > > the > > > > >>>>>>>> same way we > > > > >>>>>>>> > > allow > > > > >>>>>>>> > > > other remote-related configurations to be set on a > topic > > > > (i.e. > > > > >>>>>>>> > > > local.retention.*) - it just won't have an effect. > > > Granted, > > > > I > > > > >>>>>>>> do > > > > >>>>>>>> > believe > > > > >>>>>>>> > > we > > > > >>>>>>>> > > > should restrict the policy being changed while a > > > disablement > > > > >>>>>>>> is > > > > >>>>>>>> > ongoing. > > > > >>>>>>>> > > > > > > > >>>>>>>> > > > re: Satish and Kamal > > > > >>>>>>>> > > > > > > > >>>>>>>> > > > 104, 1 and 2 are fair asks, I will work with Doguscan > to > > > > >>>>>>>> update the KIP > > > > >>>>>>>> > > > with the information! > > > > >>>>>>>> > > > > > > > >>>>>>>> > > > Best, > > > > >>>>>>>> > > > Christo > > > > >>>>>>>> > > > > > > > >>>>>>>> > > > On Thu, 28 Mar 2024 at 10:31, Doğuşcan Namal < > > > > >>>>>>>> namal.dogus...@gmail.com > > > > >>>>>>>> > > > > > > >>>>>>>> > > > wrote: > > > > >>>>>>>> > > > > > > > >>>>>>>> > > > > Hi Satish, I will try to answer as much as I can and > > the > > > > >>>>>>>> others could > > > > >>>>>>>> > > > chime > > > > >>>>>>>> > > > > in with further details. > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > *101. For remote.log.disable.policy=delete: Does it > > > delete > > > > >>>>>>>> the remote > > > > >>>>>>>> > > log > > > > >>>>>>>> > > > > data immediately and the data in remote storage will > > not > > > > be > > > > >>>>>>>> taken > > > > >>>>>>>> > into > > > > >>>>>>>> > > > > account by any replica? That means log-start-offset > is > > > > >>>>>>>> moved to the > > > > >>>>>>>> > > > earlier > > > > >>>>>>>> > > > > local-log-start-offset.* > > > > >>>>>>>> > > > > *Exactly. RemoteLogData will be deleted > immediately. * > > > > >>>>>>>> > > > > *So before the deletion starts we move LogStart > offset > > > to > > > > >>>>>>>> > > > > LocalLogStartOffset to ensure that no RemoteLog will > > be > > > > >>>>>>>> accessed > > > > >>>>>>>> > after > > > > >>>>>>>> > > > that > > > > >>>>>>>> > > > > point.* > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > * 102. Can we update the remote.log.disable.policy > > after > > > > >>>>>>>> tiered > > > > >>>>>>>> > storage > > > > >>>>>>>> > > > is > > > > >>>>>>>> > > > > disabled on a topic?* > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > *This is a good point. I think we should not allow > > > > >>>>>>>> modifying this > > > > >>>>>>>> > > > > configuration* > > > > >>>>>>>> > > > > *because changing the policy from Deletion to Retain > > > when > > > > >>>>>>>> there is an > > > > >>>>>>>> > > > > ongoing Deletion will result in an undefined > behaviour > > > and > > > > >>>>>>>> where we > > > > >>>>>>>> > > > retain > > > > >>>>>>>> > > > > half of the remote log and delete the other half.* > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > * 103. Do we plan to add any metrics related to this > > > > >>>>>>>> feature?* > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > *Any recommendations?* > > > > >>>>>>>> > > > > *We may emit a gauge showing the enablement state > of a > > > > >>>>>>>> topic but we > > > > >>>>>>>> > > could > > > > >>>>>>>> > > > > gather that info from the logs as well.* > > > > >>>>>>>> > > > > *The total duration for remote topic deletion could > be > > > > >>>>>>>> added as well > > > > >>>>>>>> > > but > > > > >>>>>>>> > > > > this is more of a metric for the > > RemotePartitionRemover > > > > >>>>>>>> itself.* > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > *104. Please add configuration details about copier > > > thread > > > > >>>>>>>> pool, > > > > >>>>>>>> > > > expiration > > > > >>>>>>>> > > > > thread pool and the migration of the existing > > > > >>>>>>>> > > > > RemoteLogManagerScheduledThreadPool.* > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > *Will add the details.* > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > 105. How is the behaviour with topic or partition > > > deletion > > > > >>>>>>>> request > > > > >>>>>>>> > > > > handled when tiered storage disablement request is > > still > > > > >>>>>>>> being > > > > >>>>>>>> > > > > processed on a topic? > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > *If the disablement policy is Delete then a > successive > > > > >>>>>>>> topic deletion > > > > >>>>>>>> > > > > request is going to be a NOOP because RemoteLogs is > > > > already > > > > >>>>>>>> being > > > > >>>>>>>> > > > deleted.* > > > > >>>>>>>> > > > > *If the disablement policy is Retain, then we only > > moved > > > > the > > > > >>>>>>>> > > > LogStartOffset > > > > >>>>>>>> > > > > and didn't touch RemoteLogs anyway, so the delete > > topic > > > > >>>>>>>> request will > > > > >>>>>>>> > > > > result* > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > *in the initiation of RemoteLog deletion.* > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > On Tue, 26 Mar 2024 at 18:21, Kamal Chandraprakash < > > > > >>>>>>>> > > > > kamal.chandraprak...@gmail.com> wrote: > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > > Hi, > > > > >>>>>>>> > > > > > > > > > >>>>>>>> > > > > > Thanks for the KIP! Overall the KIP looks good and > > > > >>>>>>>> covered most of > > > > >>>>>>>> > > the > > > > >>>>>>>> > > > > > items. > > > > >>>>>>>> > > > > > > > > > >>>>>>>> > > > > > 1. Could you explain how the brokers will handle > the > > > > >>>>>>>> > > DisableRemoteTopic > > > > >>>>>>>> > > > > API > > > > >>>>>>>> > > > > > request? > > > > >>>>>>>> > > > > > > > > > >>>>>>>> > > > > > 2. Who will initiate the controller interaction > > > > sequence? > > > > >>>>>>>> Does the > > > > >>>>>>>> > > > > > controller listens for > > > > >>>>>>>> > > > > > topic config updates and initiate the disablement? > > > > >>>>>>>> > > > > > > > > > >>>>>>>> > > > > > -- > > > > >>>>>>>> > > > > > Kamal > > > > >>>>>>>> > > > > > > > > > >>>>>>>> > > > > > On Tue, Mar 26, 2024 at 4:40 PM Satish Duggana < > > > > >>>>>>>> > > > satish.dugg...@gmail.com > > > > >>>>>>>> > > > > > > > > > >>>>>>>> > > > > > wrote: > > > > >>>>>>>> > > > > > > > > > >>>>>>>> > > > > > > Thanks Mehari, Divij, Christo etal for the KIP. > > > > >>>>>>>> > > > > > > > > > > >>>>>>>> > > > > > > I had an initial review of the KIP and left the > > > below > > > > >>>>>>>> comments. > > > > >>>>>>>> > > > > > > > > > > >>>>>>>> > > > > > > 101. For remote.log.disable.policy=delete: > > > > >>>>>>>> > > > > > > Does it delete the remote log data immediately > and > > > the > > > > >>>>>>>> data in > > > > >>>>>>>> > > remote > > > > >>>>>>>> > > > > > > storage will not be taken into account by any > > > replica? > > > > >>>>>>>> That means > > > > >>>>>>>> > > > > > > log-start-offset is moved to the earlier > > > > >>>>>>>> local-log-start-offset. > > > > >>>>>>>> > > > > > > > > > > >>>>>>>> > > > > > > 102. Can we update the remote.log.disable.policy > > > after > > > > >>>>>>>> tiered > > > > >>>>>>>> > > storage > > > > >>>>>>>> > > > > > > is disabled on a topic? > > > > >>>>>>>> > > > > > > > > > > >>>>>>>> > > > > > > 103. Do we plan to add any metrics related to > this > > > > >>>>>>>> feature? > > > > >>>>>>>> > > > > > > > > > > >>>>>>>> > > > > > > 104. Please add configuration details about > copier > > > > >>>>>>>> thread pool, > > > > >>>>>>>> > > > > > > expiration thread pool and the migration of the > > > > existing > > > > >>>>>>>> > > > > > > RemoteLogManagerScheduledThreadPool. > > > > >>>>>>>> > > > > > > > > > > >>>>>>>> > > > > > > 105. How is the behaviour with topic or > partition > > > > >>>>>>>> deletion > > > > >>>>>>>> > request > > > > >>>>>>>> > > > > > > handled when tiered storage disablement request > is > > > > >>>>>>>> still being > > > > >>>>>>>> > > > > > > processed on a topic? > > > > >>>>>>>> > > > > > > > > > > >>>>>>>> > > > > > > ~Satish. > > > > >>>>>>>> > > > > > > > > > > >>>>>>>> > > > > > > On Mon, 25 Mar 2024 at 13:34, Doğuşcan Namal < > > > > >>>>>>>> > > > namal.dogus...@gmail.com > > > > >>>>>>>> > > > > > > > > > >>>>>>>> > > > > > > wrote: > > > > >>>>>>>> > > > > > > > > > > > >>>>>>>> > > > > > > > Hi Christo and Luke, > > > > >>>>>>>> > > > > > > > > > > > >>>>>>>> > > > > > > > I think the KRaft section of the KIP requires > > > slight > > > > >>>>>>>> > improvement. > > > > >>>>>>>> > > > The > > > > >>>>>>>> > > > > > > metadata propagation in KRaft is handled by the > > RAFT > > > > >>>>>>>> layer > > > > >>>>>>>> > instead > > > > >>>>>>>> > > of > > > > >>>>>>>> > > > > > > sending Controller -> Broker RPCs. In fact, > > KIP-631 > > > > >>>>>>>> deprecated > > > > >>>>>>>> > > these > > > > >>>>>>>> > > > > > RPCs. > > > > >>>>>>>> > > > > > > > > > > > >>>>>>>> > > > > > > > I will come up with some recommendations on > how > > we > > > > >>>>>>>> could > > > > >>>>>>>> > improve > > > > >>>>>>>> > > > that > > > > >>>>>>>> > > > > > > one but until then, @Luke please feel free to > > review > > > > >>>>>>>> the KIP. > > > > >>>>>>>> > > > > > > > > > > > >>>>>>>> > > > > > > > @Satish, if we want this to make it to Kafka > > 3.8 I > > > > >>>>>>>> believe we > > > > >>>>>>>> > > need > > > > >>>>>>>> > > > to > > > > >>>>>>>> > > > > > > aim to get the KIP approved in the following > weeks > > > > >>>>>>>> otherwise it > > > > >>>>>>>> > > will > > > > >>>>>>>> > > > > slip > > > > >>>>>>>> > > > > > > and we can not support it in Zookeeper mode. > > > > >>>>>>>> > > > > > > > > > > > >>>>>>>> > > > > > > > I also would like to better understand what is > > the > > > > >>>>>>>> community's > > > > >>>>>>>> > > > stand > > > > >>>>>>>> > > > > > for > > > > >>>>>>>> > > > > > > adding a new feature for Zookeeper since it is > > > marked > > > > as > > > > >>>>>>>> > deprecated > > > > >>>>>>>> > > > > > already. > > > > >>>>>>>> > > > > > > > > > > > >>>>>>>> > > > > > > > Thanks. > > > > >>>>>>>> > > > > > > > > > > > >>>>>>>> > > > > > > > > > > > >>>>>>>> > > > > > > > > > > > >>>>>>>> > > > > > > > On Mon, 18 Mar 2024 at 13:42, Christo Lolov < > > > > >>>>>>>> > > > christolo...@gmail.com> > > > > >>>>>>>> > > > > > > wrote: > > > > >>>>>>>> > > > > > > >> > > > > >>>>>>>> > > > > > > >> Heya, > > > > >>>>>>>> > > > > > > >> > > > > >>>>>>>> > > > > > > >> I do have some time to put into this, but to > be > > > > >>>>>>>> honest I am > > > > >>>>>>>> > > still > > > > >>>>>>>> > > > > > after > > > > >>>>>>>> > > > > > > >> reviews of the KIP itself :) > > > > >>>>>>>> > > > > > > >> > > > > >>>>>>>> > > > > > > >> After the latest changes it ought to be > > detailing > > > > >>>>>>>> both a > > > > >>>>>>>> > > Zookeeper > > > > >>>>>>>> > > > > > > approach > > > > >>>>>>>> > > > > > > >> and a KRaft approach. > > > > >>>>>>>> > > > > > > >> > > > > >>>>>>>> > > > > > > >> Do you have any thoughts on how it could be > > > > improved > > > > >>>>>>>> or > > > > >>>>>>>> > should I > > > > >>>>>>>> > > > > > start a > > > > >>>>>>>> > > > > > > >> voting thread? > > > > >>>>>>>> > > > > > > >> > > > > >>>>>>>> > > > > > > >> Best, > > > > >>>>>>>> > > > > > > >> Christo > > > > >>>>>>>> > > > > > > >> > > > > >>>>>>>> > > > > > > >> On Thu, 14 Mar 2024 at 06:12, Luke Chen < > > > > >>>>>>>> show...@gmail.com> > > > > >>>>>>>> > > > wrote: > > > > >>>>>>>> > > > > > > >> > > > > >>>>>>>> > > > > > > >> > Hi Christo, > > > > >>>>>>>> > > > > > > >> > > > > > >>>>>>>> > > > > > > >> > Any update with this KIP? > > > > >>>>>>>> > > > > > > >> > If you don't have time to complete it, I > can > > > > >>>>>>>> collaborate > > > > >>>>>>>> > with > > > > >>>>>>>> > > > you > > > > >>>>>>>> > > > > to > > > > >>>>>>>> > > > > > > work > > > > >>>>>>>> > > > > > > >> > on it. > > > > >>>>>>>> > > > > > > >> > > > > > >>>>>>>> > > > > > > >> > Thanks. > > > > >>>>>>>> > > > > > > >> > Luke > > > > >>>>>>>> > > > > > > >> > > > > > >>>>>>>> > > > > > > >> > On Wed, Jan 17, 2024 at 11:38 PM Satish > > > Duggana < > > > > >>>>>>>> > > > > > > satish.dugg...@gmail.com> > > > > >>>>>>>> > > > > > > >> > wrote: > > > > >>>>>>>> > > > > > > >> > > > > > >>>>>>>> > > > > > > >> > > Hi Christo, > > > > >>>>>>>> > > > > > > >> > > Thanks for volunteering to contribute to > > the > > > > KIP > > > > >>>>>>>> > > discussion. I > > > > >>>>>>>> > > > > > > suggest > > > > >>>>>>>> > > > > > > >> > > considering this KIP for both ZK and > KRaft > > as > > > > it > > > > >>>>>>>> will be > > > > >>>>>>>> > > > helpful > > > > >>>>>>>> > > > > > for > > > > >>>>>>>> > > > > > > >> > > this feature to be available in 3.8.0 > > running > > > > >>>>>>>> with ZK > > > > >>>>>>>> > > > clusters. > > > > >>>>>>>> > > > > > > >> > > > > > > >>>>>>>> > > > > > > >> > > Thanks, > > > > >>>>>>>> > > > > > > >> > > Satish. > > > > >>>>>>>> > > > > > > >> > > > > > > >>>>>>>> > > > > > > >> > > On Wed, 17 Jan 2024 at 19:04, Christo > > Lolov < > > > > >>>>>>>> > > > > > christolo...@gmail.com > > > > >>>>>>>> > > > > > > > > > > > >>>>>>>> > > > > > > >> > > wrote: > > > > >>>>>>>> > > > > > > >> > > > > > > > >>>>>>>> > > > > > > >> > > > Hello! > > > > >>>>>>>> > > > > > > >> > > > > > > > >>>>>>>> > > > > > > >> > > > I volunteer to get this KIP moving > > forward > > > > and > > > > >>>>>>>> > implemented > > > > >>>>>>>> > > > in > > > > >>>>>>>> > > > > > > Apache > > > > >>>>>>>> > > > > > > >> > > Kafka > > > > >>>>>>>> > > > > > > >> > > > 3.8. > > > > >>>>>>>> > > > > > > >> > > > > > > > >>>>>>>> > > > > > > >> > > > I have caught up with Mehari offline > and > > we > > > > >>>>>>>> have agreed > > > > >>>>>>>> > > that > > > > >>>>>>>> > > > > > given > > > > >>>>>>>> > > > > > > >> > Apache > > > > >>>>>>>> > > > > > > >> > > > Kafka 4.0 being around the corner we > > would > > > > >>>>>>>> like to > > > > >>>>>>>> > propose > > > > >>>>>>>> > > > > this > > > > >>>>>>>> > > > > > > feature > > > > >>>>>>>> > > > > > > >> > > > only for KRaft clusters. > > > > >>>>>>>> > > > > > > >> > > > > > > > >>>>>>>> > > > > > > >> > > > Any and all reviews and comments are > > > welcome! > > > > >>>>>>>> > > > > > > >> > > > > > > > >>>>>>>> > > > > > > >> > > > Best, > > > > >>>>>>>> > > > > > > >> > > > Christo > > > > >>>>>>>> > > > > > > >> > > > > > > > >>>>>>>> > > > > > > >> > > > On Tue, 9 Jan 2024 at 09:44, Doğuşcan > > > Namal < > > > > >>>>>>>> > > > > > > namal.dogus...@gmail.com> > > > > >>>>>>>> > > > > > > >> > > > wrote: > > > > >>>>>>>> > > > > > > >> > > > > > > > >>>>>>>> > > > > > > >> > > > > Hi everyone, any progress on the > status > > > of > > > > >>>>>>>> this KIP? > > > > >>>>>>>> > > > Overall > > > > >>>>>>>> > > > > > > looks > > > > >>>>>>>> > > > > > > >> > > good to > > > > >>>>>>>> > > > > > > >> > > > > me but I wonder whether we still need > > to > > > > >>>>>>>> support it > > > > >>>>>>>> > for > > > > >>>>>>>> > > > > > > Zookeeper > > > > >>>>>>>> > > > > > > >> > mode > > > > >>>>>>>> > > > > > > >> > > > > given that it will be deprecated in > the > > > > next > > > > >>>>>>>> 3 months. > > > > >>>>>>>> > > > > > > >> > > > > > > > > >>>>>>>> > > > > > > >> > > > > On 2023/07/21 20:16:46 "Beyene, > Mehari" > > > > >>>>>>>> wrote: > > > > >>>>>>>> > > > > > > >> > > > > > Hi everyone, > > > > >>>>>>>> > > > > > > >> > > > > > I would like to start a discussion > on > > > > >>>>>>>> KIP-950: > > > > >>>>>>>> > Tiered > > > > >>>>>>>> > > > > > Storage > > > > >>>>>>>> > > > > > > >> > > Disablement > > > > >>>>>>>> > > > > > > >> > > > > ( > > > > >>>>>>>> > > > > > > >> > > > > > > > > >>>>>>>> > > > > > > >> > > > > > > > > >>>>>>>> > > > > > > >> > > > > > > >>>>>>>> > > > > > > >> > > > > > >>>>>>>> > > > > > > > > > > >>>>>>>> > > > > > > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > > > > >>>>>>>> > > > > > > >>>>>>>> > > > > > >>>>>>>> > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-950%3A++Tiered+Storage+Disablement > > > > >>>>>>>> > > > > > > >> > > > > ). > > > > >>>>>>>> > > > > > > >> > > > > > > > > > >>>>>>>> > > > > > > >> > > > > > This KIP proposes adding the > ability > > to > > > > >>>>>>>> disable and > > > > >>>>>>>> > > > > > re-enable > > > > >>>>>>>> > > > > > > >> > tiered > > > > >>>>>>>> > > > > > > >> > > > > storage on a topic. > > > > >>>>>>>> > > > > > > >> > > > > > > > > > >>>>>>>> > > > > > > >> > > > > > Thanks, > > > > >>>>>>>> > > > > > > >> > > > > > Mehari > > > > >>>>>>>> > > > > > > >> > > > > > > > > > >>>>>>>> > > > > > > >> > > > > > > > > >>>>>>>> > > > > > > >> > > > > > > >>>>>>>> > > > > > > >> > > > > > >>>>>>>> > > > > > > > > > > >>>>>>>> > > > > > > > > > >>>>>>>> > > > > > > > > >>>>>>>> > > > > > > > >>>>>>>> > > > > > > >>>>>>>> > > > > > >>>>>>>> > > > > >>>>>>> > > > > > > > > > >
