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
