> On June 23, 2015, 5:59 p.m., Jason Gustafson wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, line > > 282 > > <https://reviews.apache.org/r/35791/diff/1/?file=990592#file990592line282> > > > > Do you think we should just move the disconnected.add() into the close > > method? > > Dong Lin wrote: > I have thought about it as well. But probabaly no. Because in > Selector.send() we put failed destinationId is put in failedSends rather than > disconnected. The reason we use failedSends is because send() and poll() in > Selector will be called asynchronously by different threads. > > Jason Gustafson wrote: > Yeah, that makes sense. I wonder if we should be using a Set internally > instead of a List? Then we wouldn't need to worry about adding to > disconnected multiple times. Guess there might be some performance impact, > but we would have an easier time ensuring correctness. > > Dong Lin wrote: > Yeah that can be useful. I am not sure if currently some id may be added > to disconnected multiple times. Even it does, this should be not a problme in > NetworkClient.handleDisconnections(). I personally prefer to keep existing > code unless there is good reason (i.e. performance or functionality) for > change. Not sure what others think about this change. > > Jason Gustafson wrote: > Seems unnecessary now that I think about it. In fact, it wouldn't hurt to > do the disconnected.add() into close() as long as we also add it to > failedSends, but that might make the code tougher to follow. Anyway, I think > the patch is fine as it is. I was just wondering if there was an easy way to > prevent this issue from popping up again in the future. > > Dong Lin wrote: > Yeah I agree with you. I will add to disconnected in close() if I were to > write this class/function from scratch. But since this is a small patch to > existing module I will make the change that solves the problem with least > dispute to make peer review smooth. > > Joel Koshy wrote: > Hi Dong - I'm not sure about your comment on multiple threads calling > send/poll. NetworkClient and Selector are both documented as being > `@nonthreadsafe` so that is probably a non-issue. The producer's sender for > e.g., does send/poll in the same (sender) thread . > > Dong Lin wrote: > Hi Joel, Jason, > > Yeah you are right. Thanks for your correction. I have moved > disconnected.add() into close() in the updated patch. Please have a look. > > Jason Gustafson wrote: > I think the only issue now is that the disconnected list can be updated > in a failed send, which technically violates the API. I doubt this would > cause any problems given current usage, but it's probably better to stay > consistent with the API. You could get around this perhaps by deferring the > close on a failed send until poll (which would introduce a nice invariant > that close is only called in poll). There may be other workarounds as well, > if you want to have a look. Otherwise, the initial patch might be the better > way to go. > > Dong Lin wrote: > Jason, could you explain why it technically violates the API? I think > when send fails, the connection is indeed closed and therefore it is OK to > update disconnected list.
The javadoc on the disconnected method says it returns the list of disconnects that finished on the last poll. The implication is that this list does not change except when poll() is called. Like I said, I don't think it would create problems with current usage, but we should at least update the javadoc if we want to allow modification of the disconnects during send() as well. I think deferring close to poll would address the issue, but maybe there are other implications. Since it seems we have to accept some ugliness conforming to this interface either way, perhaps the initial patch might be the most consistent way. - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35791/#review89010 ----------------------------------------------------------- On June 24, 2015, 1:48 a.m., Dong Lin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35791/ > ----------------------------------------------------------- > > (Updated June 24, 2015, 1:48 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2298 > https://issues.apache.org/jira/browse/KAFKA-2298 > > > Repository: kafka > > > Description > ------- > > KAFKA-2298; Client Selector can drop connections on InvalidReceiveException > without notifying NetworkClient > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/common/network/Selector.java > 4aee214b24fd990be003adc36d675f015bf22fe6 > > Diff: https://reviews.apache.org/r/35791/diff/ > > > Testing > ------- > > > Thanks, > > Dong Lin > >
