Hi Christo, Doguscan, Can you please address the comments in this mail thread and update the KIP accordingly?
Thanks, Satish. On Wed, 3 Apr 2024 at 12:30, 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 > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > >
