Hey folks, I have also synced on the KIP-578 which was doing the partition limit, to make sure the partition limit error code would be properly propagated once it is done on top of KIP-590. Let me know if you have further questions or concerns.
Boyang On Tue, Jun 23, 2020 at 5:08 PM Boyang Chen <reluctanthero...@gmail.com> wrote: > Thanks for the clarification, Colin and Ismael. Personally I also feel > Option A is better to prioritize fixing the gap. Just to be clear, the > proposed solution would be: > > 1. Bump the Metadata RPC version to return POLICY_VIOLATION. In the > application level, we should swap the error message with the actual failure > reason such as "violation of topic creation policy when attempting to auto > create internal topic through MetadataRequest." > > 2. For older Metadata RPC, return AUTHORIZATION_FAILED to fail fast. > > Will address our other discussed points as well in the KIP, let me know if > you have further questions. > > Thanks, > Boyang > > On Tue, Jun 23, 2020 at 10:41 AM Ismael Juma <ism...@juma.me.uk> wrote: > >> Option A is basically what I was thinking. But with a slight adjustment: >> >> New versions of MetadataResponse return POLICY_VIOLATION, old versions >> return AUTHORIZATION_FAILED. The latter works correctly with old Java >> clients (i.e. the client fails fast and propagates the error), I've tested >> it. Adjust new clients to treat POLICY_VIOLATION like >> AUTHORIZATION_FAILED, >> but propagate the custom error message. >> >> Ismael >> >> On Mon, Jun 22, 2020 at 11:00 PM Colin McCabe <cmcc...@apache.org> wrote: >> >> > > > > On Fri, Jun 19, 2020 at 3:18 PM Ismael Juma <ism...@juma.me.uk> >> > wrote: >> > > > > >> > > > > > Hi Colin, >> > > > > > >> > > > > > The KIP states in the Compatibility section (not Future work): >> > > > > > >> > > > > > "To support the proxy of requests, we need to build a channel >> for >> > > > > > brokers to talk directly to the controller. This part of the >> design >> > > > > > is internal change only and won’t block the KIP progress." >> > > > > > >> > > > > > I am clarifying that this is not internal only due to the >> config. >> > If we >> > > > > > say that this KIP depends on another KIP before we can merge >> > > > > > it, that's fine although it feels a bit unnecessary. >> > > > > > >> > >> > Hi Ismael, >> > >> > I didn't realize there was still a reference to the separate controller >> > channel in the "Compatibility, Deprecation, and Migration Plan" >> section. I >> > agree that it doesn't really belong there. Given that this is creating >> > confusion, I would suggest that we just drop this from the KIP entirely. >> > It really is orthogonal to what this KIP is about-- we don't need a >> > separate channel to implement redirection. >> > >> > Boyang wrote: >> > >> > > >> > > We are only opening the doors for specific internal topics (offsets, >> txn >> > > log), which I assume the client should have no possibility to mutate >> the >> > > topic policy? >> > > >> > >> > Hi Boyang, >> > >> > I think you and Ismael are talking about different scenarios. You are >> > describing the scenario where the broker is auto-creating the >> transaction >> > log topic or consumer offset topic. This scenario indeed should not >> happen >> > in a properly-configured cluster. However, Ismael is describing a >> scenario >> > where the client is auto-creating some arbitrary non-internal topic >> just by >> > sending a metadata request. >> > >> > As far as I can see, there are two solutions here: >> > >> > A. Close the hole in CreateTopicsPolicy immediately. In new versions, >> > allow MetadataResponse to return AUTHORIZATION_FAILED if we tried to >> > auto-create a topic and failed. Find some other error code to return >> for >> > existing versions. >> > >> > B. Keep the hole in CreateTopicsPolicy and add some configuration to >> allow >> > admins to gradually migrate to closing it. In practice, this probably >> > means a configuration toggle that enables direct ZK access, that starts >> off >> > as enabled. Then we can eventually default it to false and then remove >> it >> > entirely over time. >> > >> > best, >> > Colin >> > >> >
