> > Hmm... I agree that the request is invalid, but it's invalid because the > version is not supported. I guess I think UnsupportedVersionException > is a very specific exception that tells users exactly what is wrong > (aha! it's the version!) whereas InvalidRequestException is kind of a > generic catch-all exception for many different types of issue with > requests. So I think it's preferable to throw the UVE here.
That seems debatable. The broker doesn't actually know whether the unknown type is supported by a more recent version or not. UVE in that case is a guess and it could just be an invalid request. Anyway, not sure it matters too much as long as we document the behavior. The main point is that there's no reason for a properly behaving client to send a type that the broker doesn't understand. Thanks, Jason On Wed, May 10, 2017 at 4:41 PM, Colin McCabe <cmcc...@apache.org> wrote: > On Wed, May 10, 2017, at 15:54, Jason Gustafson wrote: > > Hey Colin, > > > > Thanks, I think continuing to bump the protocol makes sense. It's nice to > > be consistent with the other APIs. In the KIP, you have the following: > > > > > If the client is newer than the broker, some of the fields may show up > as > > UNKNOWN on the broker side. In this case, the filter will get an > > UnsupportedVersionException and the filter will not be applied. > > > > So the only case this would happen is if the client sent an invalid type > > for the request version, right? In that case, I would expect the client > > to raise an exception before sending the request when it sees that the > > broker does not support the needed request version. So maybe this should > it be > > an invalid request error instead perhaps? > > Hmm... I agree that the request is invalid, but it's invalid because the > version is not supported. I guess I think UnsupportedVersionException > is a very specific exception that tells users exactly what is wrong > (aha! it's the version!) whereas InvalidRequestException is kind of a > generic catch-all exception for many different types of issue with > requests. So I think it's preferable to throw the UVE here. > > I agree that the exception can be thrown by the client itself even > before sending anything over the wire. This is the case with our > existing usage of UVE, which happens when someone calls offsetsForTimes > on a pre-0.10.1 broker that is too old to support that call. The client > code throws the exception, not the broker. I expect most UVEs will be > thrown by client code in the future since most of them correspond to > "the broker has no vocabulary to even understand $NEWFEATURE." ACLs are > one case where we do have to have an additional server-side check in > case a poorly coded client is sending enum codes from the future. > > cheers, > Colin > > > > > Thanks, > > Jason > > > > > > On Wed, May 10, 2017 at 3:10 PM, Colin McCabe <cmcc...@apache.org> > wrote: > > > > > Hi Jason, > > > > > > Thanks for taking a look. > > > > > > I think it's likely that we will bump the ApiVersion of the various > > > requests if we add a new resource type, operation type, or permission > > > type. Although it may not strictly be required, bumping the version > > > number makes it a little bit clearer that the protocol is evolving and > > > clarifies the relative age of the server and client. This would be > > > helpful in a debugging situation when using broker-api-versions.sh, or > > > similar, since it would clarify that the server does/does not support > > > some particular detail of ACLs. > > > > > > However, the v1 server will still need a way of responding to a v0 > > > ListAclRequest. In that case, any type not present in v0 will be > mapped > > > to UNKNOWN, as required. This mapping could be done either on the > > > client or the server side... for simplicity's sake, it probably should > > > be done on the client side. > > > > > > The intention is to make it clear to the v0 client that "something is > > > going on" even if the v0 client can't see what the exact details are of > > > the v1 ACL. For example, the v0 client might see some ACLs on a > > > particular topic, but not be able to understand exactly what they are > > > (they are type UNKNOWN) until you upgrade your client. This would at > > > least give you some idea what was going on if a user was having trouble > > > doing something with that topic. There may be cases where we can't > even > > > provide this much (for example, if we ever implement role-based access > > > control, the v0 client simply won't have any idea about it). But we > are > > > doing what we can for compatibility. > > > > > > cheers, > > > Colin > > > > > > > > > On Wed, May 10, 2017, at 12:26, Jason Gustafson wrote: > > > > Hi Colin, > > > > > > > > Thanks for the KIP. Looks good overall. One thing I wasn't too clear > > > > about > > > > is whether new resource types, operations, or permissions require a > > > > version > > > > bump for the three new request types. On reading the proposal, it > sort of > > > > sounds like the intent is not to bump the versions and let the new > type > > > > be > > > > mapped to "unknown" on clients/brokers that don't support it. Is that > > > > correct? > > > > > > > > Thanks, > > > > Jason > > > > > > > > On Wed, May 10, 2017 at 8:24 AM, Ismael Juma <ism...@juma.me.uk> > wrote: > > > > > > > > > Thanks for the KIP, +1(binding) from me. Colin, it may make sense > to > > > start > > > > > a new thread for this as GMail is currently hiding it in the > middle of > > > the > > > > > discuss thread. Please mention the date the thread was started > along > > > with > > > > > the 3 +1s (1 binding) so far. > > > > > > > > > > Ismael > > > > > > > > > > On Sat, Apr 29, 2017 at 1:09 AM, Colin McCabe <cmcc...@apache.org> > > > wrote: > > > > > > > > > > > Hi all, > > > > > > > > > > > > I'd like to start the voting for KIP-140: Add administrative > RPCs for > > > > > > adding, deleting, and listing ACLs. This provides an API for > adding, > > > > > > deleting, and listing the access control lists (ACLs) which are > used > > > to > > > > > > control access on Kafka topics and brokers. > > > > > > > > > > > > The wiki page is here: > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > > > 140%3A+Add+administrative+RPCs+for+adding%2C+deleting% > > > > > 2C+and+listing+ACLs > > > > > > > > > > > > The previous [DISCUSS] thread: > > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg70858.html > > > > > > > > > > > > cheers, > > > > > > Colin > > > > > > > > > > > > > > >
