Hi Guozhang, Thanks for the review!
In the timeout <= 0 case, if the client should always ignore and treat > the timeout > error code as "OK", should we just return no error code in this case? The wiki behavior documentation was there, but a question was never brought up for create or delete. I treat a 0ms timeout the same as 1ms timeout, and that generally means that all valid topics will have a timeout exception. I am not sure having special behavior for <= 0 is more intuitive. I am not sure the client would always want to ignore the exception, it may want to be relayed to the user. Regardless handling on the client side should be fairly straight forward either way. I could be convinced otherwise, but I am leaning towards leaving it as is. Thanks, Grant On Tue, Jun 21, 2016 at 6:08 PM, Guozhang Wang <wangg...@gmail.com> wrote: > I see we have the similar setting for CreateTopic request timeout <= 0 as > well, so maybe it has been discussed and I simply overlooked.. otherwise my > question is for both of these cases. > > Guozhang > > On Tue, Jun 21, 2016 at 4:07 PM, Guozhang Wang <wangg...@gmail.com> wrote: > > > Thanks Grant, looks good to me overall. One minor comment below: > > > > > - The error code in the response will either contain an argument > > > validation exception or a timeout exception. If you receive a > > timeout > > > exception, because you asked for 0 timeout, you can assume the > > message was > > > valid and the topic deletion was triggered. > > > > In the timeout <= 0 case, if the client should always ignore and treat > the timeout > > error code as "OK", should we just return no error code in this case? > > > > > > Guozhang > > > > > > On Tue, Jun 21, 2016 at 8:17 AM, Grant Henke <ghe...@cloudera.com> > wrote: > > > >> Hi Ismael, > >> > >> Thanks for the review. See my responses below. > >> > >> One potential issue is that the number of topics in the response won't > >> > match the number of topics in the request. Worth checking what others > >> think > >> > of this one. > >> > >> > >> Based on the choice of how we handled duplicate topics in the create > >> topics > >> protocol, I took the same approach here. At one point create topics > would > >> disconnect because I could't return an error per topic request. In the > end > >> the preference was to return and error code even though the cardinality > >> would be different. > >> > >> 4. When requesting to delete a topic that is already marked for > >> > > > deletion, the request will wait for the wait for the timeout > and > >> > > return as > >> > > > usual > >> > > >> > Do you mean that it will wait up to the timeout until the delete is > >> > "complete" as per the definition in `6`? Or will it wait > unconditionally > >> > until the timeout expires? It would be good to make that clear. > >> > > >> > >> That's exactly what I meant. I updated the wiki. > >> > >> This could leak topic name information (as per KAFKA-3396, which was > filed > >> > by you). We would probably want to return `InvalidTopic` for the case > >> where > >> > the user doesn't have a valid `DESCRIBE TOPIC` ACL, right? > >> > >> > >> Good point. I will update the wiki and patch. > >> > >> Unauthorized requests will receive a TopicAuthorizationException if they > >> are authorized to the "Describe" Operation on the "Topic" resource. > >> Otherwise they will receive an InvalidTopicException as if the topic > does > >> not exist. > >> > >> I was wondering (and this applies to the create topic as well), is there > >> > any value in a flag that says whether the timeout expired or not? > >> > >> > >> In both the create and delete response we return a TimeoutException > error > >> code for the topics that did not "complete" in time. This allows the > >> client > >> to know which topics actions completed and which timed out. I updated > the > >> wiki to explicitly call this out in the response section. > >> > >> Thanks, > >> Grant > >> > >> On Tue, Jun 21, 2016 at 5:44 AM, Ismael Juma <ism...@juma.me.uk> wrote: > >> > >> > Thanks Grant. A few comments inline. > >> > > >> > On Mon, Jun 20, 2016 at 9:09 PM, Grant Henke <ghe...@cloudera.com> > >> wrote: > >> > > >> > > > 2. If there are multiple instructions for the same topic in one > >> > > > request the extra request will be ignored > >> > > > - This is because the list of topics is modeled server side > >> as a > >> > > set > >> > > > - Multiple deletes results in the same end goal, so handling > >> this > >> > > > error for the user should be okay > >> > > > >> > > >> > One potential issue is that the number of topics in the response won't > >> > match the number of topics in the request. Worth checking what others > >> think > >> > of this one. > >> > > >> > 4. When requesting to delete a topic that is already marked for > >> > > > deletion, the request will wait for the wait for the timeout > and > >> > > return as > >> > > > usual > >> > > >> > > >> > Do you mean that it will wait up to the timeout until the delete is > >> > "complete" as per the definition in `6`? Or will it wait > unconditionally > >> > until the timeout expires? It would be good to make that clear. > >> > > >> > > >> > > > 5. The principal must be authorized to the "Delete" Operation > on > >> the > >> > > > >> > > "Topic" resource to delete the topic. > >> > > > - Unauthorized requests will receive a > >> > TopicAuthorizationException > >> > > > >> > > >> > This could leak topic name information (as per KAFKA-3396, which was > >> filed > >> > by you). We would probably want to return `InvalidTopic` for the case > >> where > >> > the user doesn't have a valid `DESCRIBE TOPIC` ACL, right? > >> > > >> > > >> > > > - Why have a timeout at all? Deletes could take a while? > >> > > > >> > > >> > I was wondering (and this applies to the create topic as well), is > there > >> > any value in a flag that says whether the timeout expired or not? > >> > > >> > Thanks, > >> > Ismael > >> > > >> > >> > >> > >> -- > >> Grant Henke > >> Software Engineer | Cloudera > >> gr...@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke > >> > > > > > > > > -- > > -- Guozhang > > > > > > -- > -- Guozhang > -- Grant Henke Software Engineer | Cloudera gr...@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
