Hey Jiangjie, I think this is going to be very confusing that close(0) waits indefinitely and close(-1) waits for 0. I understand this appears in other apis, but it is a constant cause of bugs. Let's not repeat that mistake.
Let's make close(0) wait for 0. We don't need a way to wait indefinitely as we already have close() so having a magical constant for that is redundant. Calling close() from the I/O thread was already possible and would block indefinitely. I think trying to silently change the behavior is probably not right. I.e. if the user calls close() in the callback there is actually some misunderstanding and they need to think more, silently making this not block will hide the problem from them which is the opposite of what we want. -Jay On Thu, Mar 12, 2015 at 1:49 AM, Jiangjie Qin <j...@linkedin.com.invalid> wrote: > Hey Joe & Jay, > > Thanks for the comments on the voting thread. Since it seems we probably > will have more discussion on this, I am just replying from the discussion > thread here. > I’ve updated the KIP page to make it less like half-baked, apologize for > the rush... > > The contract in current KIP is: > 1. close() - wait until all requests either are sent or reach request > timeout. > 2. close(-1, TimeUnit.MILLISECONDS) - close immediately > 3. close(0, TimeUnit.MILLISECONDS) - equivalent to close(), i.e. Wait > until all requests are sent or reach request timeout > 4. close(5, TimeUnit.MILLISECONDS) - try the best to finish sending in 5 > milliseconds, if something went wrong, just shutdown the producer anyway, > my callback will handle the failures. > > About how we define what timeout value stands for, I actually struggled a > little bit when wrote the patch. Intuitively, close(0) should mean > immediately, however it seems that all the existing java class have this > convention of timeout=0 means no timeout or never timeout (Thread.join(0), > Object.wait(0), etc.) So here the dilemma is either we follow the > intuition or we follow the convention. What I chose is to follow the > convention but document the interface to let user be aware of the usage. > The reason is that I think producer.close() is a public interface so it > might be better to follow java convention. Whereas selector is not a > public interface that used by user, so as long as it makes sense to us, it > is less a problem to be different from java convention. That said since > consumer.poll(timeout) is also a public interface, I think it also makes > sense to make producer.close() to have the same definition of > consumer.poll(timeout). > > The main argument for keeping a timeout in close would be separating the > close timeout from request timeout, which probably makes sense. I would > guess typically the request timeout would be long (e.g. 60 seconds) > because we might want to consider retries with back off time. If we have > multiple batches in accumulator, in worst case that could take up to > several minutes to complete all the requests. But when we close a > producer, we might not want to wait for that long as it might cause some > other problem like deployment tool timeout. > > There is also a subtle difference between close(timeout) and > flush(timeout). The only purpose for flush() is to write data to the > broker, so it makes perfect sense to wait until request timeout. I think > that is why flush(timeout) looks strange. On the other hand, the top > priority for close() is to close the producer rather than flush() data, so > close(timeout) gives guarantee on bounded waiting for its main job. > > Sorry for the confusion about forceClose flag. It is not a public > interface. I mentioned it in Proposed Changes section which I thought was > supposed to provide implementation details. > > Thanks again for all the comments and suggestions! > > Jiangjie (Becket) Qin > > On 3/10/15, 8:57 PM, "Jiangjie Qin" <j...@linkedin.com> wrote: > > >The KIP page has been updated per Jay¹s comments. > >I¹d like to initiate the voting process if no further comments are > >received by tomorrow. > > > >Jiangjie (Becket) Qin > > > >On 3/8/15, 9:45 AM, "Jay Kreps" <jay.kr...@gmail.com> wrote: > > > >>Hey Jiangjie, > >> > >>Can you capture the full motivation and use cases for the feature? This > >>mentions your interest in having a way of aborting from inside the > >>Callback. But it doesn't really explain that usage or why other people > >>would want to do that. It also doesn't list the primary use case for > >>having > >>close with a bounded timeout which was to avoid blocking too long on > >>shutdown. > >> > >>-Jay > >> > >> > >> > >>On Sat, Mar 7, 2015 at 12:25 PM, Jiangjie Qin <j...@linkedin.com.invalid > > > >>wrote: > >> > >>> Hi, > >>> > >>> I just created a KIP for adding a close(timeout) to new producer. Most > >>>of > >>> the previous discussions are in KAFKA-1660 where Parth Brahmbhatt has > >>> already done a lot of work. > >>> Since this is an interface change so we are going through the KIP > >>>process. > >>> Here is the KIP link: > >>> > >>> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=5373978 > >>>2 > >>> > >>> Thanks. > >>> > >>> Jiangjie (Becket) Qin > >>> > > > >
