Re: Review Request 17649: Patch for KAFKA-1237

> On Feb. 6, 2014, 10:36 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java, line 41 > > > > > > That's right. > > > > The existing mirror maker uses Utils.abs(java.util.Ar

Re: Review Request 17649: Patch for KAFKA-1237

> On Feb. 7, 2014, 4:53 a.m., Jay Kreps wrote: > > core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java, line 25 > > > > > > I don't think we should have a list of producers. We should just have a > > single pr

Re: Review Request 17649: Patch for KAFKA-1237

> On Feb. 6, 2014, 9:43 p.m., Jay Kreps wrote: > > core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java, line 41 > > > > > > I don't think this works as the key is a byte[] so the hashCode is > > based on the o

Re: Review Request 17649: Patch for KAFKA-1237

> On Feb. 7, 2014, 4:50 a.m., Jay Kreps wrote: > > I want to second Joel's comment on Java. I think/thought the plan was to > > keep the server in scala at least for the time being. I would be in favor > > of moving the server to Java too, but I think if we want to do that we > > might need to

[jira] [Commented] (KAFKA-1241) Cryptic serde error messages in new producer

[ https://issues.apache.org/jira/browse/KAFKA-1241?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13894207#comment-13894207 ] Neha Narkhede commented on KAFKA-1241: -- Yes, it is a bug I hit during a test with lar

Re: Review Request 17836: Better error message for underflow during struct deserialization

> On Feb. 7, 2014, 4:29 a.m., Neha Narkhede wrote: > > clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java, > > line 52 > > > > > > Does it make sense to catch all Throwables so we get meaningful e

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 6, 2014, 11:48 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/controller/PartitionStateMachine.scala, lines > > 457-458 > > > > > > | should be ||. > > Neha Narkhede wrote: > No, it shouldn't. This

[jira] [Resolved] (KAFKA-1232) make TopicCommand more consistent

[ https://issues.apache.org/jira/browse/KAFKA-1232?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Jun Rao resolved KAFKA-1232. Resolution: Fixed Thanks for the review. Committed to trunk. > make TopicCommand more consistent > ---

Re: Review Request 17649: Patch for KAFKA-1237

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17649/#review33904 --- core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java

Re: Review Request 17649: Patch for KAFKA-1237

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17649/#review33903 --- I want to second Joel's comment on Java. I think/thought the plan wa

Re: Review Request 17836: Better error message for underflow during struct deserialization

> On Feb. 7, 2014, 4:29 a.m., Neha Narkhede wrote: > > clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java, > > line 52 > > > > > > Does it make sense to catch all Throwables so we get meaningful e

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 6, 2014, 11:48 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/controller/PartitionStateMachine.scala, lines > > 457-458 > > > > > > | should be ||. > > Neha Narkhede wrote: > No, it shouldn't. This

[jira] [Commented] (KAFKA-1241) Cryptic serde error messages in new producer

[ https://issues.apache.org/jira/browse/KAFKA-1241?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13894188#comment-13894188 ] Jay Kreps commented on KAFKA-1241: -- I improved the error for that case. I think the ratio

Re: Review Request 17836: Better error message for underflow during struct deserialization

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17836/#review33898 --- clients/src/main/java/org/apache/kafka/common/protocol/types/Schema

[jira] [Updated] (KAFKA-330) Add delete topic support

[ https://issues.apache.org/jira/browse/KAFKA-330?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Neha Narkhede updated KAFKA-330: Resolution: Fixed Status: Resolved (was: Patch Available) Thanks for the reviews. This is a

[jira] [Closed] (KAFKA-330) Add delete topic support

[ https://issues.apache.org/jira/browse/KAFKA-330?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Neha Narkhede closed KAFKA-330. --- > Add delete topic support > - > > Key: KAFKA-330 >

[jira] [Commented] (KAFKA-1241) Cryptic serde error messages in new producer

[ https://issues.apache.org/jira/browse/KAFKA-1241?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13894185#comment-13894185 ] Jay Kreps commented on KAFKA-1241: -- Created reviewboard https://reviews.apache.org/r/1783

[jira] [Updated] (KAFKA-1241) Cryptic serde error messages in new producer

[ https://issues.apache.org/jira/browse/KAFKA-1241?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Jay Kreps updated KAFKA-1241: - Attachment: KAFKA-1241.patch > Cryptic serde error messages in new producer > ---

Review Request 17836: Better error message for underflow during struct deserialization

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17836/ --- Review request for kafka. Bugs: KAFKA-1241 https://issues.apache.org/jira/b

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 6, 2014, 11:48 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/controller/PartitionStateMachine.scala, lines > > 94-96 > > > > > > The inner "if" is not properly indented. That is due to line wrapping

Re: Review Request 17460: Patch for 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. core/src/main/scala/kafka/con

[jira] [Commented] (KAFKA-1171) Gradle build for Kafka

[ https://issues.apache.org/jira/browse/KAFKA-1171?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893925#comment-13893925 ] Joel Koshy commented on KAFKA-1171: --- 1 - build.gradle: there should be a programmatic wa

Re: Review Request 17649: Patch for KAFKA-1237

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17649/#review33851 --- core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java

Re: Metrics in new producer

Also, here is the javadoc for this package: http://empathybox.com/kafka-metrics-javadoc/index.html?kafka/common/metrics/package-summary.html On Thu, Feb 6, 2014 at 12:51 PM, Jay Kreps wrote: > Hey guys, > > I wanted to kick off a quick discussion of metrics with respect to the new > producer an

Re: Review Request 17649: Patch for KAFKA-1237

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17649/#review33845 --- core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java

Re: Metrics in new producer

Jay, Not a kafka dev (yet), but I really like having Coda metrics in Kafka since it simplifies the integration with dropwizard services, which I use a ton of. Using coda metrics in kafka means that they should automatically be surfaced on the dropwizard metrics REST endpoint as well as JMX and any

Re: Config for new clients (and server)

Hey David, Great questions. The Configurable interface was meant to use with plugins. Originally we had two of these in the producer, Serializer and Partitioner but we removed them so now it is unused. However I think we will likely have several in the consumer so I will leave it there for now. T

Metrics in new producer

Hey guys, I wanted to kick off a quick discussion of metrics with respect to the new producer and consumer (and potentially the server). At a high level I think there are three approaches we could take: 1. Plain vanilla JMX 2. Use Coda Hale (AKA Yammer) Metrics 3. Do our own metrics (with JMX as

Re: Config for new clients (and server)

Jay, thanks for the clarification. I think I understand it a bit better now, and It all pretty much makes sense. See inline Also, slightly OT, but what are Configurable and AbstractConfig#getConfiguredInstance for? I don't see anything using them. On 2/6/14 11:46 AM, Jay Kreps wrote: Hey Dav

[jira] [Updated] (KAFKA-330) Add delete topic support

[ https://issues.apache.org/jira/browse/KAFKA-330?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Neha Narkhede updated KAFKA-330: Attachment: KAFKA-330_2014-02-06_11:37:48.patch > Add delete topic support > --

[jira] [Commented] (KAFKA-330) Add delete topic support

[ https://issues.apache.org/jira/browse/KAFKA-330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893707#comment-13893707 ] Neha Narkhede commented on KAFKA-330: - Updated reviewboard https://reviews.apache.org/r

Re: Review Request 17460: Patch for KAFKA-330

--- 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

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 6, 2014, 7:15 p.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/controller/TopicDeletionManager.scala, line 330 > > > > > > Shall we check if isTopicDeletionHalted(topic) here at the first place? > >

Re: Review Request 17649: Patch for KAFKA-1237

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17649/#review33830 --- Ship it! Ship It! - Guozhang Wang On Feb. 4, 2014, 9:12 p.m., Ne

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 6, 2014, 7:15 p.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/controller/TopicDeletionManager.scala, line 330 > > > > > > Shall we check if isTopicDeletionHalted(topic) here at the first place? No,

[jira] [Comment Edited] (KAFKA-1171) Gradle build for Kafka

[ https://issues.apache.org/jira/browse/KAFKA-1171?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893674#comment-13893674 ] Neha Narkhede edited comment on KAFKA-1171 at 2/6/14 7:18 PM: --

[jira] [Comment Edited] (KAFKA-1171) Gradle build for Kafka

[ https://issues.apache.org/jira/browse/KAFKA-1171?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893674#comment-13893674 ] Neha Narkhede edited comment on KAFKA-1171 at 2/6/14 7:17 PM: --

Re: Review Request 17460: Patch for KAFKA-330

> 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 cha

[jira] [Commented] (KAFKA-1171) Gradle build for Kafka

[ https://issues.apache.org/jira/browse/KAFKA-1171?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893674#comment-13893674 ] Neha Narkhede commented on KAFKA-1171: -- +1 on the latest patch. Few minor comments -

Re: Review Request 17460: Patch for 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

[jira] Subscription: outstanding kafka patches

Issue Subscription Filter: outstanding kafka patches (86 issues) The list of outstanding kafka patches Subscriber: kafka-mailing-list Key Summary KAFKA-1238 New producer hangs in a loop detecting metadata for auto created topics https://issues.apache.org/jira/browse/KAFKA-123

Re: Config for new clients (and server)

Hey Guozhang, You have to accept the boolean true to support convenient programmatic configuration (the example I showed). You have to support taking a string to allow injecting config from a config file or external application config system. E.g. in the case where you supply a Properties instance

[jira] [Commented] (KAFKA-1171) Gradle build for Kafka

[ https://issues.apache.org/jira/browse/KAFKA-1171?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893636#comment-13893636 ] Jun Rao commented on KAFKA-1171: These commands no longer exist in the latest patch. The n

[jira] [Commented] (KAFKA-1237) Add mirror maker using 08 consumer and 09 producer

[ https://issues.apache.org/jira/browse/KAFKA-1237?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893631#comment-13893631 ] Neha Narkhede commented on KAFKA-1237: -- Bump! Can someone review this please? > Add

[jira] [Commented] (KAFKA-330) Add delete topic support

[ https://issues.apache.org/jira/browse/KAFKA-330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893621#comment-13893621 ] Neha Narkhede commented on KAFKA-330: - Updated reviewboard https://reviews.apache.org/r

Re: Review Request 17460: Patch for KAFKA-330

--- 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

[jira] [Updated] (KAFKA-330) Add delete topic support

[ https://issues.apache.org/jira/browse/KAFKA-330?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Neha Narkhede updated KAFKA-330: Attachment: KAFKA-330_2014-02-06_10:29:31.patch > Add delete topic support > --

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 5, 2014, 10:23 p.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/controller/TopicDeletionManager.scala, line 131 > > > > > > It is an exception case if topicsTobeDeleted.!contains(topics). > > Neha Na

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 5, 2014, 10:23 p.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/controller/ControllerChannelManager.scala, line > > 215 > > > > > > Instead of checking the replicaId == -1 case here, I feel it is bet

[jira] [Updated] (KAFKA-1171) Gradle build for Kafka

[ https://issues.apache.org/jira/browse/KAFKA-1171?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Neha Narkhede updated KAFKA-1171: - Priority: Blocker (was: Major) > Gradle build for Kafka > -- > >

[jira] [Updated] (KAFKA-330) Add delete topic support

[ https://issues.apache.org/jira/browse/KAFKA-330?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Neha Narkhede updated KAFKA-330: Priority: Blocker (was: Major) > Add delete topic support > - > >

Re: Config for new clients (and server)

I made a typo in my previous email, I meant to say "not allowing", i.e. if a config' type is Boolean, do not accept "true" as the value. On Thu, Feb 6, 2014 at 8:30 AM, Jay Kreps wrote: > Guozhang, > > In spirit I agree with you, but it is really annoying if I can't say >props.put("config.a

[jira] [Commented] (KAFKA-1171) Gradle build for Kafka

[ https://issues.apache.org/jira/browse/KAFKA-1171?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893576#comment-13893576 ] Neha Narkhede commented on KAFKA-1171: -- Tested patch 14. Here are some review comment

[jira] [Updated] (KAFKA-330) Add delete topic support

[ https://issues.apache.org/jira/browse/KAFKA-330?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Neha Narkhede updated KAFKA-330: Attachment: KAFKA-330_2014-02-06_09:42:38.patch > Add delete topic support > --

[jira] [Commented] (KAFKA-330) Add delete topic support

[ https://issues.apache.org/jira/browse/KAFKA-330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893572#comment-13893572 ] Neha Narkhede commented on KAFKA-330: - Updated reviewboard https://reviews.apache.org/r

Re: Review Request 17460: Patch for KAFKA-330

--- 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

Re: Review Request 17460: Patch for KAFKA-330

> 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 cha

[jira] [Commented] (KAFKA-1215) Rack-Aware replica assignment option

[ https://issues.apache.org/jira/browse/KAFKA-1215?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893535#comment-13893535 ] Joris Van Remoortere commented on KAFKA-1215: - [~junrao] could you please look

Re: Config for new clients (and server)

Hey David, You raise a couple points, let me give the rationale and see if you are convinced. 1. Why not reuse commons config? This is exactly the right question for application programming. We could save ourselves a few hundred lines of code we would maintain by just adding a library dependency,

Re: Config for new clients (and server)

Joel, Ah, I actually don't think the internal usage is a problem for *us*. We just use config in one place, whereas it gets set in 1000s of apps, so I am implicitly optimizing for the application interface. I agree that we can add getters and setters on the ProducerConfig if we like. Basically I w

Re: Config for new clients (and server)

Guozhang, In spirit I agree with you, but it is really annoying if I can't say props.put("config.a", 45) props.put("config.b", true) This is especially true because config will actually come from elsewhere and may have the appropriate type so in reality you end up with props.put("config.

[jira] [Updated] (KAFKA-330) Add delete topic support

[ https://issues.apache.org/jira/browse/KAFKA-330?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Neha Narkhede updated KAFKA-330: Attachment: KAFKA-330_2014-02-06_07:48:40.patch > Add delete topic support > --

[jira] [Commented] (KAFKA-330) Add delete topic support

[ https://issues.apache.org/jira/browse/KAFKA-330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893459#comment-13893459 ] Neha Narkhede commented on KAFKA-330: - Updated reviewboard against branch trunk > Add

Re: Review Request 17460: Patch for 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

Re: Review Request 17460: Patch for KAFKA-330

--- 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

Re: Config for new clients (and server)

The declarative approach strikes me as a bit odd. Why not put the precondition logic in a POJO wrapper? Also, why reinvent commons-configuration? It's got a nice API and tons of people use it. public class ProducerConfig extends org.apache.commons.configuration. AbstractConfiguration { /**