Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-20 Thread Jun Rao
That's true. However, in general, timeout is not going to be enforced precisely due to other things like GCs. Thanks, Jun On Fri, Mar 20, 2015 at 4:39 PM, Guozhang Wang wrote: > With this approach close(timeout > 0) will not actually close by the > elapsed timeout right? This semantics mismatc

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-20 Thread Guozhang Wang
With this approach close(timeout > 0) will not actually close by the elapsed timeout right? This semantics mismatch is a bit concerning to me.. Guozhang On Fri, Mar 20, 2015 at 2:37 PM, Jun Rao wrote: > Yes, that probably is simpler. Basically, close() and close(timeout) will > always wait for

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-20 Thread Jun Rao
Yes, that probably is simpler. Basically, close() and close(timeout) will always wait for the sender thread to complete. Close(0) will just initiate the shutdown of the sender thread, but not waiting for the thread to complete. Thanks, Jun On Fri, Mar 20, 2015 at 11:29 AM, Jiangjie Qin wrote:

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-20 Thread Jiangjie Qin
That’s a very good point. Currently if timeout > 0, it will wait up to timeout for the sender thread to complete. After that it will do a force close but not block. Maybe we should make it to be: If timeout > 0, wait up to timeout for the sender thread to complete. After that, make a force close b

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-20 Thread Jun Rao
Got it. Just to clarify, does close(timeout) always wait for the sender thread to complete? Thanks, Jun On Fri, Mar 20, 2015 at 7:41 AM, Jiangjie Qin wrote: > I think the we agreed that we are going to log an error and block. By > doing this we can make sure the error log to be checked by user

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-20 Thread Jiangjie Qin
I think the we agreed that we are going to log an error and block. By doing this we can make sure the error log to be checked by user in all cases. If we silently replace close() to close(0) in sender thread, in some cases such as send error during a normal close(), user might not notice something

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-19 Thread Joel Koshy
close can probably just getCurrentThread and check if == senderThread. I'm actually not sure from this thread if there was clear agreement on whether it should change close(timeout)/close() to close(0) or if it should log an error and block up to the timeout. On Thursday, March 19, 2015, Jun Rao

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-19 Thread Jun Rao
So in (1), if a close() or close(timeout) is called from a callback, we will just turn that into a close(0)? Implementation wise, how do we know whether a close() call is made from the sender thread or not? Thanks, Jun On Wed, Mar 18, 2015 at 2:13 PM, Jiangjie Qin wrote: > It looks we have ano

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-19 Thread Jiangjie Qin
OK, we are going to follow (1) then. It looks the KIP does not need further updates. Should we resume the voting process? On 3/19/15, 3:28 PM, "Jay Kreps" wrote: >Yeah guys, I think this is one where the cure is worse than the disease. >Let's just have the two close methods, that is confusing en

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-19 Thread Jay Kreps
Yeah guys, I think this is one where the cure is worse than the disease. Let's just have the two close methods, that is confusing enough. Adding a third won't help. I understand that having a few microseconds before the thread shuts down could be unexpected but I think there is nothing inherently w

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-19 Thread Jiangjie Qin
Hi Neha, As Joel said, (1) and (2) provides almost exact same function as far as I can see. I actually have no strong preference. The only difference is that (2) provides the close() semantic meaning of making sure all the resources have gone away, at the cost of adding another abort() interface.

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-19 Thread Joel Koshy
(1) should work, but as Jun suggested earlier in the thread it is slightly misleading. The (intuitive) post-condition of "close" is that the producer has shutdown - i.e., its sender thread, closed its metrics, serializer/deserializer, etc. That is not necessarily a post-condition of "close(0)" alth

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-19 Thread Neha Narkhede
I'm in favor of (1) for the sake of simplicity and as Jay mentions to reduce the number of different APIs. Can you explain when (1) does not work? On Wed, Mar 18, 2015 at 2:52 PM, Jay Kreps wrote: > Personally I'm in favor of (1) just to reduce the number of different APIs. > People will find th

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-18 Thread Jay Kreps
Personally I'm in favor of (1) just to reduce the number of different APIs. People will find the difference between abort and close subtle and confusing and the only instance where you want it is this somewhat unusual case you guys are pursuing, right? -Jay On Wed, Mar 18, 2015 at 2:13 PM, Jiangj

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-18 Thread Jiangjie Qin
It looks we have another option and are now deciding between the following two interfaces: 1. Close() + close(timeout) - timeout could be either positive or zero. - only close(0) can be called from sender thread 2. Close() + abort() + close(timeout) - timeout can either be positive or zero

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-17 Thread Jiangjie Qin
Hi Jun, Yes, as Guozhang said, the main reason we set a flag is because close(0) is expected to be called by sender thread itself. If we want to maintain the semantic meaning of close(), one alternative is to have an abort() method does the same thing as close(0) except cleanup. And in close(timeo

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-16 Thread Guozhang Wang
Yeah in this sense the sender thread will not exist immediately in the close(0) call, but will only terminate after the current response batch has been processed, as will the producer instance itself. There is a reason for this though: for a clean shutdown the caller thread has to wait for the sen

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-16 Thread Jun Rao
Hmm, does that mean that after close(0), the sender thread is not necessary gone? Normally, after closing an entity, we expect all internal threads associated with the entity are shut down completely. Thanks, Jun On Mon, Mar 16, 2015 at 3:18 PM, Jiangjie Qin wrote: > Hi Jun, > > Close(0) will

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-16 Thread Jiangjie Qin
Hi Jun, Close(0) will set two flags in sender. Running=false and a newly added forceClose=true. It will also set accumulator.closed=true so no further producer.send() will succeed. The sender thread will finish executing all the callbacks in current batch of responses, then it will see the forceCl

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-16 Thread Jun Rao
How does close(0) work if it's called from the sender thread? If close(0) needs to wait for the sender thread to join, wouldn't this cause a deadlock? Thanks, Jun On Mon, Mar 16, 2015 at 2:26 PM, Jiangjie Qin wrote: > Thanks Guozhang. It wouldn’t be as thoroughly considered without > discussin

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-16 Thread Jiangjie Qin
Thanks Guozhang. It wouldn’t be as thoroughly considered without discussing with you :) Jiangjie (Becket) Qin On 3/16/15, 1:07 PM, "Guozhang Wang" wrote: >Thanks Jiangjie, > >After talking to you offline on this, I have been convinced and changed my >preference to blocking. The immediate shutdo

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-16 Thread Guozhang Wang
Thanks Jiangjie, After talking to you offline on this, I have been convinced and changed my preference to blocking. The immediate shutdown approach does have some unsafeness in some cases. Guozhang On Mon, Mar 16, 2015 at 11:50 AM, Jiangjie Qin wrote: > It looks that the problem we want to sol

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-16 Thread Jiangjie Qin
It looks that the problem we want to solve and the purpose we want to achieve is: If user uses close() in callback, we want to let user be aware that they should use close(0) instead of close() in the callback. We have agreed that we will have an error log to inform user about this mis-usage. The

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-16 Thread Guozhang Wang
HI Jiangjie, As far as I understand calling close() in the ioThread is not common, as it may only trigger when we saw some non-retriable error. Hence when user run their program it is unlikely that close() will be triggered and problem will be detected. So it seems to me that from the error detect

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-16 Thread Jiangjie Qin
It seems there are two options we can choose from when close() is called from sender thread (callback): 1. Log an error and close the producer using close(-1) 2. Log an error and block. (Throwing an exception will not work because we catch all the exception thrown from user callback. It will just l

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-16 Thread Jiangjie Qin
Agree. Since throwing exception when close() is called in callback won’t work because we are catching all the exceptions from callback, blocking might be the only option we have here. Jiangjie (Becket) Qin On 3/15/15, 11:56 AM, "Jay Kreps" wrote: >Cool. > >I think blocking is good or alternatel

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-15 Thread Guozhang Wang
Yeah I agree we should not silently change the behavior of the function with the given parameters; and I would prefer error-logging-and-shutdown over blocking when close(>0) is used, since as Neha suggested blocking would also not proceed with sending any data, bu will just let users to realize the

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-15 Thread Neha Narkhede
> > And I also agree it is better if we can make producer block when > close() is called from sender thread so user will notice something went > wrong. This isn't a great experience either. Why can't we just throw an exception for a behavior we know is incorrect and we'd like the user to know. Bl

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-15 Thread Jay Kreps
Cool. I think blocking is good or alternately throwing an exception directly from close(). Basically I would just worry about subtly doing something slightly different from what the user asked for as it will be hard to notice that behavior difference. -Jay On Sat, Mar 14, 2015 at 5:48 PM, Jiangj

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-14 Thread Jiangjie Qin
Hi Jay, I have modified the KIP as you suggested. I thinks as long as we have consistent define for timeout across Kafka interface, there would be no problem. And I also agree it is better if we can make producer block when close() is called from sender thread so user will notice something went wr

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-14 Thread Jay Kreps
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 indefini

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-14 Thread Jiangjie Qin
Sounds reasonable. I have updated the KIP page accordingly. On 3/12/15, 10:16 PM, "Guozhang Wang" wrote: >3) I think this is fine. >4) Hmm, error-message-only may NOT be better than blocking, as with the >code written with close(>=0), it will likely to just pollute the logs with >repeating error

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-12 Thread Guozhang Wang
3) I think this is fine. 4) Hmm, error-message-only may NOT be better than blocking, as with the code written with close(>=0), it will likely to just pollute the logs with repeating errors until someone gets notified. How about error-message-and-close(-1), i.e. record the error and force shutdown?

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-12 Thread Jiangjie Qin
Hi Jay, Currently I can see use case for 4). We’ve seen some application in LinkedIn having issue of undeployment failure because of timeout, it was because many Kafka consumers were shutdown at the same time and caused serialized consumer rebalance until retries are exhausted. I guess in practice

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-12 Thread Jay Kreps
Hey Jiangjie, Thanks for the awesome summary, I think that really captures it. I agree, the question is exactly whether there would be a motivational use case we can brainstorm for those additional use cases? -Jay On Thu, Mar 12, 2015 at 4:24 PM, Jiangjie Qin wrote: > Hi folks, > > Since there

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-12 Thread Jiangjie Qin
Hi folks, Since there are actually a long interleaved discussions in KAFKA-1660 and KAFKA-1659, I summarized them in the rejected alternatives sections - the section exists for a reason and I shouldn’t have always put None there. . . It looks we took a while to agree upon the current interface. W

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-12 Thread Jiangjie Qin
Hey Guozhang, Thanks for the comments. I updated the page as suggested. For 3), that’s right, I put this in java doc. Do you think we need to reject value other than -1? For 4), I think user will notice this easily because the thread will block and producer is not going to shutdown. About using cl

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-12 Thread Guozhang Wang
Hi Becket, Some comments on the wiki page: 1. There are a few typos, for example "multivations", "wiithin". 2. I think the main motivation could just be "current close() needs to block on flushing all buffered data, however there are scenarios in which producers would like to close without block

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-12 Thread Jiangjie Qin
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.

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-10 Thread Jiangjie Qin
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" wrote: >Hey Jiangjie, > >Can you capture the full motivation and use cases for the feature? This >ment

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-09 Thread Jiangjie Qin
Hi Jay, I have updated the KIP as you suggested. -Jiangjie (Becket) Qin On 3/8/15, 9:45 AM, "Jay Kreps" 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

Re: [DISCUSSION] KIP-15 close(timeout) for producer

2015-03-08 Thread Jay Kreps
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 havin

[DISCUSSION] KIP-15 close(timeout) for producer

2015-03-07 Thread Jiangjie Qin
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.or