Hi Mickael, Thanks for the revisions. It looks good overall.
best, Colin On Mon, Nov 11, 2019, at 03:45, Mickael Maison wrote: > Hi all, > > If there's no more feedback, I'll open a vote in the next few days. > > Thanks > > On Wed, Nov 6, 2019 at 11:35 AM Mickael Maison <mickael.mai...@gmail.com> > wrote: > > > > Hi, > > > > Thanks Colin for the feedback. Edo and I have updated the KIP > > accordingly. Can you take another look? > > > > > > On Tue, Oct 22, 2019 at 12:20 AM Colin McCabe <cmcc...@apache.org> wrote: > > > > > > Hi Mickael, > > > > > > We don't have any official way for brokers to join the cluster other than > > > showing up and registering themselves in ZK. Similarly, we don't have > > > any way of removing brokers from the cluster other than simply removing > > > them and removing their znodes from ZooKeeper. > > > > > > If we wanted to change this, it seems like it would be a really big step. > > > We would need public, stable APIs for both of these things. Or at least > > > for the removal thing, which is currently automatic and doesn't require > > > any action on the part of the administrator. Administrators would have to > > > be retrained to do this whenever shrinking the cluster. > > > We cannot tell people to modify ZK directly for this. > > > > > > To be honest, I don't think reworking broker registration is worth it for > > > this change. I think we could pretty easily have placeholder values for > > > the missing replicas like -1, -2, -3, etc. and just fill them in whenever > > > a new broker comes online. This may be slightly more complex to > > > implement, but it greatly simplifies what users have to do. > > > > > > It is true that filling in -1, -2, -3, etc. will not preserve rack > > > placement information. But this is kind of a more general problem that > > > we should probably solve separately. After placement, a lot of placement > > > information disappears and is not accessible to reassignment. Since > > > reassignment is becoming more and more important, we should make an > > > effort to preserve this information. Since that would be a big change, > > > it's probably best to do separately, however. > > > > > > The "rejected alternatives" section says that adding an option to > > > CreateTopicsRequest to allow users to opt-in to the new behavior "felt > > > too complex." But I think this could use a little clarification. Adding > > > a new boolean to the createTopics command is actually fairly simple from > > > the perspective of a developer. But it adds another thing for end-users > > > to think about when using the software. It's also not clear how many > > > users would take advantage of this. I think that's the reason people > > > were not in favor of it, not a general feeling of complexity. Adding > > > more configuration options is often simple to implement, and making > > > things "just work" is often a little more complex. But we should prefer > > > the latter, most of the time at least. I think this is what you meant > > > here, but it would be good to clarify. > > > > > > "Rejected alternatives" also talks about an error code and an error > > > message when the replication is not up to full strength. But this was > > > removed, right? We should clarify that no error code is returned in this > > > case, and the CreateTopicsResponse returns the true number of replicas > > > that was created, in case the client is interested in this information. > > > Returning an error code would certainly cause problems for a lot of > > > users, who use all().get() to verify that all the topics have been > > > successfully created. > > > > > > best, > > > Colin > > > > > > > > > On Mon, Oct 21, 2019, at 09:50, Mickael Maison wrote: > > > > Thanks Stanislav and Colin for the feedback. > > > > > > > > I've updated the KIP to make it simpler. > > > > It's not updating the CreateTopics/CreatePartitions RPCs anymore. I've > > > > kept the broker setting so admins can keep the current behaviour but > > > > simplified it to be either enabled or disabled. > > > > > > > > I've also kept the observed_brokers nodes in Zookeeper. I can't think > > > > of a better alternative to keep track of the expected brokers. The > > > > other option would be to perform the extra replica creation > > > > asynchronously (driven by the controller when a broker joins the > > > > cluster) but that feels a lot more complicated for this specific use > > > > case. > > > > > > > > I've also made it explicit that at least "min.insync.replicas" brokers > > > > have to be online to allow topic/partition creation. > > > > > > > > Thanks > > > > > > > > On Mon, Mar 25, 2019 at 1:17 PM Mickael Maison > > > > <mickael.mai...@gmail.com> wrote: > > > > > > > > > > Thanks Colin for the feedback. > > > > > > > > > > The idea was to allow both users and administrator to decide if they > > > > > wanted to opt-in and if so under what conditions. > > > > > > > > > > Maybe we could do something simpler and just allow the creation if at > > > > > least min-in-sync replicas are available? That should not require > > > > > changes to the protocol and while this might not cover all possible > > > > > use cases, that would still cover the use cases we've listed in the > > > > > KIP. That would also tie in with existing semantics/guarantees > > > > > (min-in-sync). > > > > > > > > > > Thanks > > > > > > > > > > On Tue, Feb 26, 2019 at 5:40 PM Colin McCabe <cmcc...@apache.org> > > > > > wrote: > > > > > > > > > > > > Hi Mickael, > > > > > > > > > > > > I don't think adding CREATED_UNDER_REPLICATED as an error code > > > > > > makes sense. It is not an error condition, as described here. > > > > > > > > > > > > > Updates to the Decommissioning brokers section in the > > > > > > > documentation > > > > > > > will mention that if a broker id is never to be reused then its > > > > > > > corresponding node in zookeeper > > > > > > > /brokers/observed_ids will need to be removed manually > > > > > > > > > > > > I don't think it's acceptable to ask admins to manually modify > > > > > > ZooKeeper here. In general the ZK changes seem kind of like a hack > > > > > > -- perhaps we should drop it from the proposal for now. > > > > > > > > > > > > Perhaps we could even somehow do all of this in a custom > > > > > > CreateTopicPolicy? That would avoid the need for RPC changes, new > > > > > > configuration knobs, etc. > > > > > > > > > > > > best, > > > > > > Colin > > > > > > > > > > > > > > > > > > On Tue, Dec 18, 2018, at 08:43, Mickael Maison wrote: > > > > > > > Hi, > > > > > > > > > > > > > > We have submitted a KIP to handle topics and partitions creation > > > > > > > when > > > > > > > a cluster is not fully available: > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-409%3A+Allow+creating+under-replicated+topics+and+partitions > > > > > > > > > > > > > > As always, we welcome feedback and suggestions. > > > > > > > > > > > > > > Thanks > > > > > > > Mickael and Edoardo > > > > > > > > > > > >
