You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Guozhang Wang <wa...@gmail.com> on 2015/06/07 06:49:46 UTC
Review Request 35187: Fix KAFKA-2253
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35187/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-2253
https://issues.apache.org/jira/browse/KAFKA-2253
Repository: kafka
Description
-------
Double check on whether the key is still correlated to the watchers in removeKey function
Diffs
-----
core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
Diff: https://reviews.apache.org/r/35187/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 35187: Fix KAFKA-2253
Posted by Guozhang Wang <wa...@gmail.com>.
> On June 7, 2015, 6:08 p.m., Jiangjie Qin wrote:
> > Is it still possible that in tryCompleteElseWatch after we get the watcher from watchersForKey() but before we call watchers.watch(), that watcher get removed from purgatory? If so, is it possible that operation got lost?
Ack.
> On June 7, 2015, 6:08 p.m., Jiangjie Qin wrote:
> > core/src/main/scala/kafka/server/DelayedOperation.scala, lines 258-265
> > <https://reviews.apache.org/r/35187/diff/1/?file=980465#file980465line258>
> >
> > Can we put this entire part into tryCompleteWatched/purgeComplete? This code block can be simplified to:
> > inWriteLock(removeWatchersLock) {
> > if (watchersForKey.get(key) == this)
> > watchersForKey.remove(key)
> > }
I feel it is better to keep it in a separate function as it is referred in two places, because when we change the logic in the future we will not forget either one of the functions.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35187/#review86972
-----------------------------------------------------------
On June 7, 2015, 4:49 a.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35187/
> -----------------------------------------------------------
>
> (Updated June 7, 2015, 4:49 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2253
> https://issues.apache.org/jira/browse/KAFKA-2253
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Double check on whether the key is still correlated to the watchers in removeKey function
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
>
> Diff: https://reviews.apache.org/r/35187/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 35187: Fix KAFKA-2253
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35187/#review86972
-----------------------------------------------------------
Is it still possible that in tryCompleteElseWatch after we get the watcher from watchersForKey() but before we call watchers.watch(), that watcher get removed from purgatory? If so, is it possible that operation got lost?
core/src/main/scala/kafka/server/DelayedOperation.scala
<https://reviews.apache.org/r/35187/#comment139187>
Can we put this entire part into tryCompleteWatched/purgeComplete? This code block can be simplified to:
inWriteLock(removeWatchersLock) {
if (watchersForKey.get(key) == this)
watchersForKey.remove(key)
}
- Jiangjie Qin
On June 7, 2015, 4:49 a.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35187/
> -----------------------------------------------------------
>
> (Updated June 7, 2015, 4:49 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2253
> https://issues.apache.org/jira/browse/KAFKA-2253
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Double check on whether the key is still correlated to the watchers in removeKey function
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
>
> Diff: https://reviews.apache.org/r/35187/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 35187: Fix KAFKA-2253
Posted by Guozhang Wang <wa...@gmail.com>.
> On June 7, 2015, 6:03 a.m., Onur Karaman wrote:
> > core/src/main/scala/kafka/server/DelayedOperation.scala, line 262
> > <https://reviews.apache.org/r/35187/diff/1/?file=980465#file980465line262>
> >
> > Forgot to mention that the commented out code should be removed.
Ack.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35187/#review86951
-----------------------------------------------------------
On June 7, 2015, 4:49 a.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35187/
> -----------------------------------------------------------
>
> (Updated June 7, 2015, 4:49 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2253
> https://issues.apache.org/jira/browse/KAFKA-2253
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Double check on whether the key is still correlated to the watchers in removeKey function
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
>
> Diff: https://reviews.apache.org/r/35187/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 35187: Fix KAFKA-2253
Posted by Onur Karaman <ok...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35187/#review86951
-----------------------------------------------------------
core/src/main/scala/kafka/server/DelayedOperation.scala
<https://reviews.apache.org/r/35187/#comment139163>
Forgot to mention that the commented out code should be removed.
- Onur Karaman
On June 7, 2015, 4:49 a.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35187/
> -----------------------------------------------------------
>
> (Updated June 7, 2015, 4:49 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2253
> https://issues.apache.org/jira/browse/KAFKA-2253
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Double check on whether the key is still correlated to the watchers in removeKey function
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
>
> Diff: https://reviews.apache.org/r/35187/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 35187: Fix KAFKA-2253
Posted by Onur Karaman <ok...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35187/#review86950
-----------------------------------------------------------
Ship it!
Ship It!
- Onur Karaman
On June 7, 2015, 4:49 a.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35187/
> -----------------------------------------------------------
>
> (Updated June 7, 2015, 4:49 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2253
> https://issues.apache.org/jira/browse/KAFKA-2253
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Double check on whether the key is still correlated to the watchers in removeKey function
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
>
> Diff: https://reviews.apache.org/r/35187/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>