I think I'm convinced your way for exposing the exception out of the client, let's leave it as is.
Guozhang On Thu, Jun 23, 2016 at 8:31 AM, Grant Henke <ghe...@cloudera.com> wrote: > 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 > -- -- Guozhang
