Hi David, DA1: It's a point I considered, especially being able to cordon a whole broker. With the current proposal you just need to set cordoned.log.dirs to the same value as log.dirs. That does not seem excessively difficult.
DA2: I did consider using a new metadata record like we do for fence/unfence. Since I don't expect cordoned log dirs to change very frequently, and the size should be small, I opted to reuse the BrokerRegistrationRecord metadata record. At the moment I guess it was mostly for the convenience while prototyping. Semantically it probably makes sense to have separate records. Your further point suggest design changes in the mechanism as a whole, so let's discuss these first and we can return to the exact metadata records after. I find the idea of having dedicated RPCs interesting. One of the reasons I piggybacked on the heartbeating process is for the mapping between log directory names and their UUIDs. Currently the mapping only exists on each broker instance. So if we wanted a dedicated RPC, we would first need to change how we manage the log directory to UUID mappings. I guess this could be done via the BrokerRegistration API. I'm not sure about storing additional metadata (reason, timestamp). We currently don't do that for any operations (AlterPartitionReassignments, UnregisterBroker). Typically these are stored in the tools used by the operators to drive these operations. You bring another interesting point about the ability to cordon brokers/log directories while they are offline. It's not something the current proposal supports. I'm not sure how useful this would turn out it practice. In experience is that brokers are mostly online, so I'd expect the need to do so relatively rare. This also kind of loops back to KAFKA-17094 [0] and the discussion Gantigmaa started on the dev list [1] about being able to identify the ids of offline (but still registered) brokers. DA3: With the current proposal, I don't see a reason why you would want to disable the new behavior. If you don't want to use it, you have nothing to do. It's opt-in as you need to set cordoned.log.dirs on some brokers to get the new behavior. If you don't want it anymore, you should unset cordoned.log.dirs. Can you explain why this would not work? DA4: Yes 0: https://issues.apache.org/jira/browse/KAFKA-17094 1: https://lists.apache.org/thread/1rrgbhk43d85wobcp0dqz6mhpn93j9yo Thanks, Mickael On Sun, Jul 14, 2024 at 10:37 AM Kamal Chandraprakash <kamal.chandraprak...@gmail.com> wrote: > > Hi Mickael, > > In the BrokerHearbeatRequest.json, the flexibleVersions are bumped from > "0+" to "1+". Is it a typo? > > > On Fri, Jul 12, 2024 at 11:42 PM David Arthur <mum...@gmail.com> wrote: > > > Mickael, thanks for the KIP! I think this could be quite a useful feature. > > > > DA1: Having to know each of the log dirs for a broker seems a bit > > inconvenient for cases where we want to cordon off a whole broker. I do > > think having the ability to cordon off a specific log dir is useful for > > JBOD, but I imagine a common case might be to cordon off the whole broker. > > > > DA2: Looks like the new "cordoned.log.dirs" can be configured statically > > and updated dynamically per-broker. What do you think about a new metadata > > record and RPC instead of using a config? From my understanding, the > > BrokerRegistration/Heartbeat is more about the lifecycle of a broker > > whereas cordoning a broker is an operator driven action. It might make > > sense to have a separate record for this. We could include additional > > fields like a timestamp, a reason/comment field (e.g., "decommissioning", > > "disk failure", "new broker" etc), stuff like that. > > > > This would also allow cordoning to be done while a broker is offline or > > before it has been provisioned. Not sure how likely that is, but might be > > useful? > > > > DA3: Can we consider having a configuration to enable/disable the new > > replica placer behavior? This would be separate from the new > > MetadataVersion for the RPC/record changes. > > > > DA4: In the Motivation section, you mention the cluster expansion scenario. > > For this scenario, is the expectation that the operator will cordon off the > > existing full brokers so placements only happen on the new brokers? > > > > Cheers, > > David > > > > On Fri, Jul 12, 2024 at 8:53 AM Mickael Maison <mickael.mai...@gmail.com> > > wrote: > > > > > Hi Kamal, > > > > > > Thanks for taking a look at the KIP! > > > > > > I briefly considered that option initially but I found it not very > > > practical once you have more than a few cordoned log directories. > > > I find your example is already not very easy to read, and it only has > > > 2 entries. Also if the configuration is at the cluster level it'sis > > > not easy to see if a broker has all its log directories cordoned, and > > > you still need to describe a specific broker's configuration to find > > > the "name" of a log directory you want to cordon. > > > > > > I think an easy way to get an overall view of the cordoned log > > > directories/brokers will be via the kafka-log-dirs.sh tool. I am also > > > considering adding metrics like we have today for LogDirectoryOffline > > > to ease monitoring. > > > > > > Thanks, > > > Mickael > > > > > > On Thu, Jul 11, 2024 at 8:41 PM Kamal Chandraprakash > > > <kamal.chandraprak...@gmail.com> wrote: > > > > > > > > Hi Mickael, > > > > > > > > Thanks for the KIP! > > > > > > > > This is a useful feature which helps to decommission the nodes by > > > > essentially > > > > creating a new replica exclude broker list. > > > > > > > > To cordon a list of brokers, we have to apply the config on each of the > > > > broker nodes > > > > and similarly to see the list of cordoned brokers, we have to either > > > query > > > > individual broker > > > > config in the cluster or query each of the broker log directory. > > > > > > > > Can we move the configuration from the broker to cluster level? (eg) > > > > > > > > bin/kafka-configs.sh --bootstrap-server localhost:9092 > > --broker-defaults > > > > --alter --add-config > > > > cordoned.log.dirs=[broker.id.0]:[log.dir.0],[broker.id.1]:[log.dir.1] > > > > > > > > This will provide a consistent view of the list of cordoned brokers in > > > the > > > > cluster. > > > > > > > > -- > > > > Kamal > > > > > > > > On Wed, Jul 10, 2024 at 7:53 PM Mickael Maison < > > mickael.mai...@gmail.com > > > > > > > > wrote: > > > > > > > > > Hi Luke, > > > > > > > > > > 4. You're right this scenario can happen. In this case I think the > > > > > broker should enforce its new state and not create the replica as all > > > > > its log directories are now cordoned. The replica will be offline and > > > > > an administrator would need to reassign it to another broker. I > > expect > > > > > most users will rely on kafka-configs.sh to manage the cordoned log > > > > > directories instead of updating the broker properties, so it should > > > > > not be a common issue. > > > > > > > > > > 6. To resolve this issue, the user can uncordon a log directory and > > > > > retry an operation that triggers the creation of the internal topics. > > > > > For example start a consumer using a group should make the broker > > > > > retry creating the __consumer_offsets topic. > > > > > > > > > > Thanks, > > > > > Mickael > > > > > > > > > > On Wed, Jul 10, 2024 at 4:14 PM Mickael Maison < > > > mickael.mai...@gmail.com> > > > > > wrote: > > > > > > > > > > > > Hi Chia-Ping, > > > > > > > > > > > > Question 1) Yes that's a good summary. I'd also add that managing > > > > > > cordoned log directories is intended to be done by cluster > > > > > > administrators who also know about operations in-progress or > > planned > > > > > > such as scaling or adding/removing log directories. In practice you > > > > > > can't expect users to provide "good" explicit partition assignments > > > as > > > > > > they are not aware of the cluster operations. > > > > > > > > > > > > Question 2) I don't see an actual question :) > > > > > > > > > > > > Question 3) My current proposal is that in that case, brokers will > > > > > > ignore the preferred log directory and place the new replica on a > > log > > > > > > directory that is not cordoned. To use AlterReplicaLogDirs you need > > > > > > ALTER permission on the CLUSTER resource. In most environments > > users > > > > > > don't have permissions to use that API. Or if they do, I'd expect > > > them > > > > > > to also have ALTER_CONFIGS on the BROKER resource and be able to > > > > > > change the cordoned log directories configuration. > > > > > > > > > > > > Question 4) If we don't include the CordonedLogDirs field in > > > > > > BrokerRegistrationRequest as you said there's a small window when > > the > > > > > > controller would not know if a broker can be used for assignments. > > If > > > > > > all log directories are cordoned the controller should not use that > > > > > > broker for assignments. > > > > > > > > > > > > Question 5) In KRaft mode offline brokers are still registered, and > > > > > > can be used for partition assignment. So we need to persist the > > > > > > cordoned log directories too to exclude brokers that don't have > > > > > > uncordoned log directories. > > > > > > > > > > > > Thanks, > > > > > > Mickael > > > > > > > > > > > > > > > > > > On Wed, Jul 10, 2024 at 12:25 PM Luke Chen <show...@gmail.com> > > > wrote: > > > > > > > > > > > > > > Hi Mickael, > > > > > > > > > > > > > > Thanks for the response. > > > > > > > > > > > > > > > 4. Cordoned log directories are persisted to the metadata log > > > via the > > > > > > > RegisterBrokerRecord, BrokerRegistrationChangeRecord records. If > > a > > > > > > > broker is offline, the controller will use the latest known state > > > of > > > > > > > the broker to determine the broker's cordoned log directories. > > I've > > > > > > > added a sentence clarifying this point. > > > > > > > > > > > > > > OK, so if the broker A goes offline, and the controller is in > > > "fenced" > > > > > > > state, without any cordoned log dirs, then some topic created and > > > > > assigned > > > > > > > to broker A. Later, broker A starts up with all its log dirs > > > cordoned > > > > > > > configured. At this situation, will the broker A create the > > > partitions? > > > > > > > > > > > > > > > 6. I'm leaning towards considering that scenario a > > configuration > > > > > > > error. If all log directories are cordoned before the internal > > > topics > > > > > > > are created, then the broker will not be able to create them. > > This > > > > > > > seems like a pretty strange scenario, where it's the first time > > you > > > > > > > start a broker, you've cordoned all its log directory, and the > > > > > > > internal topics (offsets and transactions) have not yet been > > > created > > > > > > > in the rest of the cluster. > > > > > > > > > > > > > > Yes, I agree that this should be a configuration error. > > > > > > > So the follow-up question is: Suppose users encounter this issue, > > > and > > > > > how > > > > > > > could they resolve it? > > > > > > > Uncordon the log dir dynamically using kafka-configs.sh? Will the > > > > > > > uncordoning config change recreate the partitions we didn't > > create > > > > > earlier > > > > > > > because of log dir cordoned? > > > > > > > > > > > > > > > The metadata log is different (not managed by LogManager), so I > > > think > > > > > > > it should always be created regardless if its log directory is > > > > > > > cordoned or not. > > > > > > > > > > > > > > I agree we should treat "__cluster_metadata" differently. > > > > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > Luke > > > > > > > > > > > > > > > > > > > > > On Wed, Jul 10, 2024 at 12:42 AM Mickael Maison < > > > > > mickael.mai...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > Hi Luke, > > > > > > > > > > > > > > > > 2. isCordoned() is a new method on LogDirDescription. It does > > not > > > > > take > > > > > > > > any arguments. It just returns true if this log directory the > > > > > > > > LogDirDescription represents is cordoned. > > > > > > > > > > > > > > > > 3. Sorry that was a typo. This method will only return a log > > > > > directory > > > > > > > > that is not cordoned. Fixed > > > > > > > > > > > > > > > > 4. Cordoned log directories are persisted to the metadata log > > > via the > > > > > > > > RegisterBrokerRecord, BrokerRegistrationChangeRecord records. > > If > > > a > > > > > > > > broker is offline, the controller will use the latest known > > > state of > > > > > > > > the broker to determine the broker's cordoned log directories. > > > I've > > > > > > > > added a sentence clarifying this point. > > > > > > > > > > > > > > > > 5. Yes a log directory can be uncordoned. You can either update > > > the > > > > > > > > properties file and restart the broker or dynamically change > > the > > > > > value > > > > > > > > at runtime using kafka-configs. I've added a paragraph about it > > > in > > > > > the > > > > > > > > KIP. > > > > > > > > > > > > > > > > 6. I'm leaning towards considering that scenario a > > configuration > > > > > > > > error. If all log directories are cordoned before the internal > > > topics > > > > > > > > are created, then the broker will not be able to create them. > > > This > > > > > > > > seems like a pretty strange scenario, where it's the first time > > > you > > > > > > > > start a broker, you've cordoned all its log directory, and the > > > > > > > > internal topics (offsets and transactions) have not yet been > > > created > > > > > > > > in the rest of the cluster. > > > > > > > > The metadata log is different (not managed by LogManager), so I > > > think > > > > > > > > it should always be created regardless if its log directory is > > > > > > > > cordoned or not. > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Mickael > > > > > > > > > > > > > > > > On Tue, Jul 9, 2024 at 3:48 PM Chia-Ping Tsai < > > > chia7...@gmail.com> > > > > > wrote: > > > > > > > > > > > > > > > > > > hi Mickael > > > > > > > > > > > > > > > > > > That is totally a good idea, but I have a question about the > > > > > > > > implementation > > > > > > > > > > > > > > > > > > Do we consider making pluggable ReplicaPlacer (KIP-660) first > > > and > > > > > then > > > > > > > > add > > > > > > > > > another impl of ReplicaPlacer to offer cordon mechanism? > > Noted > > > that > > > > > > > > > `ReplicaPlacer` can implement Reconfigurable to get updated > > at > > > > > runtime. > > > > > > > > > That is similar to KIP-1066 - change cordoned.log.dirs > > through > > > > > configs > > > > > > > > > update. > > > > > > > > > > > > > > > > > > The benefit is to let users have their optimized policy for > > > > > specific > > > > > > > > > scenario. Also, it can avoid that we add more and more > > > mechanism > > > > > to our > > > > > > > > > code base. Of course we can merge the mechanism which can be > > > used > > > > > by 99% > > > > > > > > > users :smile > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > Chia-Ping > > > > > > > > > > > > > > > > > > > > > > > > > > > Luke Chen <show...@gmail.com> 於 2024年7月9日 週二 下午9:07寫道： > > > > > > > > > > > > > > > > > > > Hi Mickael, > > > > > > > > > > > > > > > > > > > > Thanks for the KIP! > > > > > > > > > > This is a long waiting feature for many users! > > > > > > > > > > > > > > > > > > > > Questions: > > > > > > > > > > 1. I think piggyback the "BrokerHeartbeatRequest" to > > forward > > > the > > > > > > > > corden log > > > > > > > > > > dir to controller makes sense to me. > > > > > > > > > > We already did similar things for fence, controller > > shutdown, > > > > > failed > > > > > > > > log > > > > > > > > > > dir...etc. > > > > > > > > > > > > > > > > > > > > 2. In the admin API, what parameters will the new added > > > > > isCordoned() > > > > > > > > method > > > > > > > > > > take? > > > > > > > > > > > > > > > > > > > > 3. In the KIP, we said: > > > > > > > > > > "defaultDir(): This method will not return the Uuid of a > > log > > > > > directory > > > > > > > > that > > > > > > > > > > is not cordoned." > > > > > > > > > > --> It's hard to understand. Does that mean we will only > > > return > > > > > > > > cordoned > > > > > > > > > > log dir? > > > > > > > > > > From the current java doc of the interface, it doesn't look > > > > > right: > > > > > > > > > > "Get the default directory for new partitions placed in a > > > given > > > > > > > > broker." > > > > > > > > > > > > > > > > > > > > 4. Currently, if a broker is registered and then go > > offline. > > > In > > > > > this > > > > > > > > state, > > > > > > > > > > the controller will still distribute partitions to this > > > broker. > > > > > > > > > > So, if now, the broker get startup with "cordoned.log.dirs" > > > set, > > > > > what > > > > > > > > will > > > > > > > > > > happen? > > > > > > > > > > Will the newly assigned partitions be created successfully > > or > > > > > not? > > > > > > > > > > > > > > > > > > > > 5. I think after a log dir get cordoned, we can always > > > uncordon > > > > > it, > > > > > > > > right? > > > > > > > > > > I think we should mention it in the KIP. > > > > > > > > > > > > > > > > > > > > 6. If a broker is startup with "cordoned.log.dirs" set, and > > > does > > > > > that > > > > > > > > mean > > > > > > > > > > the internal topics partitions (ex: __consumer_offsets) > > > cannot be > > > > > > > > created, > > > > > > > > > > either? > > > > > > > > > > Also, if this log dir is happen to be the metadata log dir, > > > what > > > > > will > > > > > > > > > > happen to the metadata topic creation? > > > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > Luke > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Jul 9, 2024 at 12:12 AM Mickael Maison < > > > > > > > > mickael.mai...@gmail.com> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > Thanks for taking a look. > > > > > > > > > > > > > > > > > > > > > > - Yes you're right, I meant AlterPartitionReassignments. > > > Fixed. > > > > > > > > > > > - That's a good idea. I was expecting users to discover > > > > > cordoned log > > > > > > > > > > > directories by describing broker configurations. But > > being > > > > > able to > > > > > > > > > > > also get this information when describing log directories > > > makes > > > > > > > > sense. > > > > > > > > > > > I've added that to the KIP. > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > Mickael > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Jul 5, 2024 at 8:05 AM Haruki Okada < > > > > > ocadar...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > Thank you for the KIP. > > > > > > > > > > > > The motivation sounds make sense to me. > > > > > > > > > > > > > > > > > > > > > > > > I have a few questions: > > > > > > > > > > > > > > > > > > > > > > > > - [nits] "AlterPartitions request" in Error handling > > > section > > > > > is > > > > > > > > > > > > "AlterPartitionReassignments request" actually, right? > > > > > > > > > > > > - Don't we need to include cordoned information in > > > > > DescribeLogDirs > > > > > > > > > > > response > > > > > > > > > > > > too? Some tools (e.g. CruiseControl) need to have a way > > > to > > > > > know > > > > > > > > which > > > > > > > > > > > > broker/log-dirs are cordoned to generate partition > > > > > reassignment > > > > > > > > > > proposal. > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > > > > > 2024年7月4日(木) 22:57 Mickael Maison < > > > mickael.mai...@gmail.com > > > > > >: > > > > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > > > I'd like to start a discussion on KIP-1066 that > > > introduces > > > > > a > > > > > > > > > > mechanism > > > > > > > > > > > > > to cordon log directories and brokers. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1066%3A+Mechanism+to+cordon+brokers+and+log+directories > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > Mickael > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > ======================== > > > > > > > > > > > > Okada Haruki > > > > > > > > > > > > ocadar...@gmail.com > > > > > > > > > > > > ======================== > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > David Arthur > >
