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 <gu...@linkedin.com> on 2014/01/17 00:02:31 UTC
Re: Review Request 16360: Address comments from Neha,
and record error messages if leader brokers are not available since this
should not happen then
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16360/
-----------------------------------------------------------
(Updated Jan. 16, 2014, 11:02 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Address comments from Neha, and record error messages if leader brokers are not available since this should not happen then
Bugs: KAFKA-1188
https://issues.apache.org/jira/browse/KAFKA-1188
Repository: kafka
Description (updated)
-------
KAFKA-1188.v6
KAFKA-1188.v5.1
KAFKA-1188.v5
Dummy
KAFKA-1188.v4
KAFKA-1188.v3
KAFKA-1118.v2
KAFKA-1188.v1
Diffs (updated)
-----
core/src/main/scala/kafka/cluster/Partition.scala 1087a2e91c86e36a2494a95913a3ec2daf238287
core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 483559aa64726c51320d18b64a1b48f8fb2905a0
core/src/main/scala/kafka/server/ReplicaManager.scala f9d10d385cee49a1e3be8c82e3ffa22ef87a8fd6
Diff: https://reviews.apache.org/r/16360/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 16360: Address comments from Neha,
and record error messages if leader brokers are not available since this
should not happen then
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16360/#review32147
-----------------------------------------------------------
core/src/main/scala/kafka/controller/ReplicaStateMachine.scala
<https://reviews.apache.org/r/16360/#comment60853>
Need to change this comment accordingly.
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/16360/#comment60852>
present => be present
now => not
- Jun Rao
On Jan. 16, 2014, 11:02 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16360/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2014, 11:02 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1188
> https://issues.apache.org/jira/browse/KAFKA-1188
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1188.v6
>
>
> KAFKA-1188.v5.1
>
>
> KAFKA-1188.v5
>
>
> Dummy
>
>
> KAFKA-1188.v4
>
>
> KAFKA-1188.v3
>
>
> KAFKA-1118.v2
>
>
> KAFKA-1188.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 1087a2e91c86e36a2494a95913a3ec2daf238287
> core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 483559aa64726c51320d18b64a1b48f8fb2905a0
> core/src/main/scala/kafka/server/ReplicaManager.scala f9d10d385cee49a1e3be8c82e3ffa22ef87a8fd6
>
> Diff: https://reviews.apache.org/r/16360/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 16360: Address comments from Neha,
and record error messages if leader brokers are not available since this
should not happen then
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Jan. 18, 2014, 2:17 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, line 425
> > <https://reviews.apache.org/r/16360/diff/6/?file=425126#file425126line425>
> >
> > "aborted the become-follower state change"...
> >
> > Also, I'm not sure this shutting down check is doing what we really want.
> > i.e., the replica fetcher manager may be shutdown right after this, and we
> > could still add fetchers. But that's a separate issue, and I may have
> > misread the code.
> >
> >
I agree with you, this check does not actually avoid adding fetchers while at the same time shutting down fetcher manager. Will file another jira for this.
> On Jan. 18, 2014, 2:17 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, line 376
> > <https://reviews.apache.org/r/16360/diff/6/?file=425126#file425126line376>
> >
> > I'm wondering if it would make the code cleaner/clearer if you move all the
> > code below that deals with partitionsToMakeFollower to this block of the if
> > statement.
> >
We use the current pattern because we want to do adding-fetcher and truncating log in batch, moving them into the if statement will revert to the one-by-one pattern.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16360/#review32242
-----------------------------------------------------------
On Jan. 16, 2014, 11:02 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16360/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2014, 11:02 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1188
> https://issues.apache.org/jira/browse/KAFKA-1188
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1188.v6
>
>
> KAFKA-1188.v5.1
>
>
> KAFKA-1188.v5
>
>
> Dummy
>
>
> KAFKA-1188.v4
>
>
> KAFKA-1188.v3
>
>
> KAFKA-1118.v2
>
>
> KAFKA-1188.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 1087a2e91c86e36a2494a95913a3ec2daf238287
> core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 483559aa64726c51320d18b64a1b48f8fb2905a0
> core/src/main/scala/kafka/server/ReplicaManager.scala f9d10d385cee49a1e3be8c82e3ffa22ef87a8fd6
>
> Diff: https://reviews.apache.org/r/16360/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 16360: Address comments from Neha,
and record error messages if leader brokers are not available since this
should not happen then
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16360/#review32242
-----------------------------------------------------------
core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/16360/#comment60970>
Would be better to move this comment to above the method and say what it
returns and what it means there.
core/src/main/scala/kafka/controller/ReplicaStateMachine.scala
<https://reviews.apache.org/r/16360/#comment60972>
Clearer to say: "to all the remaining (alive) replicas" of the partition.
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/16360/#comment60981>
I'm wondering if it would make the code cleaner/clearer if you move all the
code below that deals with partitionsToMakeFollower to this block of the if
statement.
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/16360/#comment60982>
"aborted the become-follower state change"...
Also, I'm not sure this shutting down check is doing what we really want.
i.e., the replica fetcher manager may be shutdown right after this, and we
could still add fetchers. But that's a separate issue, and I may have
misread the code.
- Joel Koshy
On Jan. 16, 2014, 11:02 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16360/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2014, 11:02 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1188
> https://issues.apache.org/jira/browse/KAFKA-1188
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1188.v6
>
>
> KAFKA-1188.v5.1
>
>
> KAFKA-1188.v5
>
>
> Dummy
>
>
> KAFKA-1188.v4
>
>
> KAFKA-1188.v3
>
>
> KAFKA-1118.v2
>
>
> KAFKA-1188.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 1087a2e91c86e36a2494a95913a3ec2daf238287
> core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 483559aa64726c51320d18b64a1b48f8fb2905a0
> core/src/main/scala/kafka/server/ReplicaManager.scala f9d10d385cee49a1e3be8c82e3ffa22ef87a8fd6
>
> Diff: https://reviews.apache.org/r/16360/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 16360: Patch for KAFKA-1188
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16360/
-----------------------------------------------------------
(Updated Feb. 12, 2014, 1:48 a.m.)
Review request for kafka.
Bugs: KAFKA-1188
https://issues.apache.org/jira/browse/KAFKA-1188
Repository: kafka
Description (updated)
-------
Rebased on delete-topics
Diffs (updated)
-----
core/src/main/scala/kafka/cluster/Partition.scala 1087a2e91c86e36a2494a95913a3ec2daf238287
core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 5e016d5d2bbef7fc33fb80c9a4a90ec5d39c16d0
core/src/main/scala/kafka/server/ReplicaManager.scala 21bba48affb38ff6504e02535d3cc56258f45b60
Diff: https://reviews.apache.org/r/16360/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 16360: Patch for KAFKA-1188
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16360/
-----------------------------------------------------------
(Updated Feb. 12, 2014, 1:58 a.m.)
Review request for kafka.
Bugs: KAFKA-1188
https://issues.apache.org/jira/browse/KAFKA-1188
Repository: kafka
Description
-------
Rebased on delete-topics
Diffs
-----
core/src/main/scala/kafka/cluster/Partition.scala 1087a2e91c86e36a2494a95913a3ec2daf238287
core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 5e016d5d2bbef7fc33fb80c9a4a90ec5d39c16d0
core/src/main/scala/kafka/server/ReplicaManager.scala 21bba48affb38ff6504e02535d3cc56258f45b60
Diff: https://reviews.apache.org/r/16360/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 16360: Patch for KAFKA-1188
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16360/#review34406
-----------------------------------------------------------
Ship it!
Only minor comments
core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/16360/#comment64521>
remove extra TopicAndPartition import
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/16360/#comment64495>
could you please wrap the long line here?
- Neha Narkhede
On Feb. 12, 2014, 1:58 a.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16360/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2014, 1:58 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1188
> https://issues.apache.org/jira/browse/KAFKA-1188
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Rebased on delete-topics
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 1087a2e91c86e36a2494a95913a3ec2daf238287
> core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 5e016d5d2bbef7fc33fb80c9a4a90ec5d39c16d0
> core/src/main/scala/kafka/server/ReplicaManager.scala 21bba48affb38ff6504e02535d3cc56258f45b60
>
> Diff: https://reviews.apache.org/r/16360/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 16360: Patch for KAFKA-1188
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16360/
-----------------------------------------------------------
(Updated Feb. 12, 2014, 1:58 a.m.)
Review request for kafka.
Bugs: KAFKA-1188
https://issues.apache.org/jira/browse/KAFKA-1188
Repository: kafka
Description
-------
Rebased on delete-topics
Diffs
-----
core/src/main/scala/kafka/cluster/Partition.scala 1087a2e91c86e36a2494a95913a3ec2daf238287
core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 5e016d5d2bbef7fc33fb80c9a4a90ec5d39c16d0
core/src/main/scala/kafka/server/ReplicaManager.scala 21bba48affb38ff6504e02535d3cc56258f45b60
Diff: https://reviews.apache.org/r/16360/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 16360: Patch for KAFKA-1188
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16360/
-----------------------------------------------------------
(Updated Feb. 12, 2014, 1:49 a.m.)
Review request for kafka.
Bugs: KAFKA-1188
https://issues.apache.org/jira/browse/KAFKA-1188
Repository: kafka
Description
-------
Rebased on delete-topics
Diffs
-----
core/src/main/scala/kafka/cluster/Partition.scala 1087a2e91c86e36a2494a95913a3ec2daf238287
core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 5e016d5d2bbef7fc33fb80c9a4a90ec5d39c16d0
core/src/main/scala/kafka/server/ReplicaManager.scala 21bba48affb38ff6504e02535d3cc56258f45b60
Diff: https://reviews.apache.org/r/16360/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 16360: Patch for KAFKA-1188
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16360/
-----------------------------------------------------------
(Updated Jan. 21, 2014, 10:01 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Patch for KAFKA-1188
Bugs: KAFKA-1188
https://issues.apache.org/jira/browse/KAFKA-1188
Repository: kafka
Description (updated)
-------
KAFKA-1188.v7
KAFKA-1188.v6
KAFKA-1188.v5.1
KAFKA-1188.v5
Dummy
KAFKA-1188.v4
KAFKA-1188.v3
KAFKA-1118.v2
KAFKA-1188.v1
Diffs (updated)
-----
core/src/main/scala/kafka/cluster/Partition.scala 1087a2e91c86e36a2494a95913a3ec2daf238287
core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 483559aa64726c51320d18b64a1b48f8fb2905a0
core/src/main/scala/kafka/server/ReplicaManager.scala f9d10d385cee49a1e3be8c82e3ffa22ef87a8fd6
Diff: https://reviews.apache.org/r/16360/diff/
Testing
-------
Thanks,
Guozhang Wang