On Feb. 8, 2014, 2:43 a.m., Joel Koshy wrote:
Already checked-in so this is really a follow-up review.
My overall take on the implementation is that it is (perhaps - because I'm
not 100 percent sure myself) complex mainly to handle corner cases which are
rare but I think recoverable.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Feb. 8, 2014, 7:07 p.m.)
Review request for kafka.
Bugs: KAFKA-330
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/#review33961
---
Already checked-in so this is really a follow-up review.
My
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Feb. 6, 2014, 3:49 p.m.)
Review request for kafka.
Bugs: KAFKA-330
On Feb. 5, 2014, 10:34 p.m., Jun Rao wrote:
Some high level comments.
1. While most of the replica states are now managed in ReplicaStateMachine,
there are a few still managed in TopicDeletionManager through
haltedTopicsForDeletion and topicDeletionInProgress. It probably would be
On Feb. 5, 2014, 10:23 p.m., Guozhang Wang wrote:
In the follow-up patch that serialize all the admin tasks in the back
ground thread, I would suggest switching away from using the callbacks to
trigger state change while executing the process but depending on some ZK
path change, as
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Feb. 6, 2014, 5:42 p.m.)
Review request for kafka.
Bugs: KAFKA-330
On Feb. 5, 2014, 10:23 p.m., Guozhang Wang wrote:
core/src/main/scala/kafka/controller/ControllerChannelManager.scala, line
215
https://reviews.apache.org/r/17460/diff/8/?file=470128#file470128line215
Instead of checking the replicaId == -1 case here, I feel it is better
to
On Feb. 5, 2014, 10:23 p.m., Guozhang Wang wrote:
core/src/main/scala/kafka/controller/TopicDeletionManager.scala, line 131
https://reviews.apache.org/r/17460/diff/8/?file=470134#file470134line131
It is an exception case if topicsTobeDeleted.!contains(topics).
Neha Narkhede
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Feb. 6, 2014, 6:29 p.m.)
Review request for kafka.
Bugs: KAFKA-330
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/#review33813
---
core/src/main/scala/kafka/controller/KafkaController.scala
On Feb. 5, 2014, 10:23 p.m., Guozhang Wang wrote:
In the follow-up patch that serialize all the admin tasks in the back
ground thread, I would suggest switching away from using the callbacks to
trigger state change while executing the process but depending on some ZK
path change, as
On Feb. 6, 2014, 7:15 p.m., Guozhang Wang wrote:
core/src/main/scala/kafka/controller/TopicDeletionManager.scala, line 330
https://reviews.apache.org/r/17460/diff/11/?file=470964#file470964line330
Shall we check if isTopicDeletionHalted(topic) here at the first place?
No, because
On Feb. 6, 2014, 7:15 p.m., Guozhang Wang wrote:
core/src/main/scala/kafka/controller/TopicDeletionManager.scala, line 330
https://reviews.apache.org/r/17460/diff/11/?file=470964#file470964line330
Shall we check if isTopicDeletionHalted(topic) here at the first place?
Neha
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Feb. 6, 2014, 7:37 p.m.)
Review request for kafka.
Bugs: KAFKA-330
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/#review33858
---
Ship it!
Some minor comments below.
On Feb. 6, 2014, 11:48 p.m., Jun Rao wrote:
core/src/main/scala/kafka/controller/PartitionStateMachine.scala, lines
94-96
https://reviews.apache.org/r/17460/diff/12/?file=471029#file471029line94
The inner if is not properly indented.
That is due to line wrapping
On Feb. 6,
On Feb. 6, 2014, 11:48 p.m., Jun Rao wrote:
core/src/main/scala/kafka/controller/PartitionStateMachine.scala, lines
457-458
https://reviews.apache.org/r/17460/diff/12/?file=471029#file471029line457
| should be ||.
Neha Narkhede wrote:
No, it shouldn't. This is set union
On Feb. 6, 2014, 11:48 p.m., Jun Rao wrote:
core/src/main/scala/kafka/controller/PartitionStateMachine.scala, lines
457-458
https://reviews.apache.org/r/17460/diff/12/?file=471029#file471029line457
| should be ||.
Neha Narkhede wrote:
No, it shouldn't. This is set union
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Feb. 5, 2014, 5:31 p.m.)
Review request for kafka.
Bugs: KAFKA-330
On Feb. 5, 2014, 2:50 a.m., Joel Koshy wrote:
core/src/main/scala/kafka/controller/ControllerChannelManager.scala, line
304
https://reviews.apache.org/r/17460/diff/7/?file=462654#file462654line304
You mean just put all in a single StopReplicaRequest? If so, any reason
not to
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/#review33711
---
In the follow-up patch that serialize all the admin tasks in the
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/#review33699
---
Some high level comments.
1. While most of the replica states are
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/#review33668
---
I haven't finished reviewing but will continue when I get time. I
On Feb. 5, 2014, 2:50 a.m., Joel Koshy wrote:
core/src/main/scala/kafka/controller/ControllerChannelManager.scala, line
220
https://reviews.apache.org/r/17460/diff/7/?file=462654#file462654line220
Why not only limit to the topic-partitions relevant to this
leaderAndIsrRequest?
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Feb. 1, 2014, 10:58 p.m.)
Review request for kafka.
Bugs: KAFKA-330
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Jan. 31, 2014, 10:19 p.m.)
Review request for kafka.
Bugs:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Feb. 1, 2014, 1:45 a.m.)
Review request for kafka.
Bugs: KAFKA-330
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/#review33179
---
A few high level comments:
1. Do we need to block adding
On Jan. 30, 2014, 1:33 a.m., Jun Rao wrote:
A few high level comments:
1. Do we need to block adding partitions when a topic is being deleted?
2. StopReplica callback: When the callback is created, we can probably let
it reference a local val for replica id. This way, we can have
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Jan. 28, 2014, 7:06 p.m.)
Review request for kafka.
Bugs: KAFKA-330
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
Review request for kafka.
Bugs: KAFKA-330
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Jan. 28, 2014, 7:13 p.m.)
Review request for kafka.
Bugs: KAFKA-330
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Jan. 28, 2014, 7:15 p.m.)
Review request for kafka.
Bugs: KAFKA-330
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Jan. 28, 2014, 9:25 p.m.)
Review request for kafka.
Bugs: KAFKA-330
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/#review33046
---
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/#review33056
---
In all, I would vote for marking the deletion as failed when some
On Jan. 28, 2014, 11:16 p.m., Guozhang Wang wrote:
In all, I would vote for marking the deletion as failed when some brokers
are in-available, but with this I still feel the delete-topic logic is
pretty complicated interleaving with other operations. Since in most
operational
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Jan. 29, 2014, 1:38 a.m.)
Review request for kafka.
Bugs: KAFKA-330
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17460/
---
(Updated Jan. 29, 2014, 6:01 a.m.)
Review request for kafka.
Bugs: KAFKA-330
40 matches
Mail list logo
