[ https://issues.apache.org/jira/browse/KAFKA-657?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13543277#comment-13543277 ]

Jay Kreps commented on KAFKA-657:
---------------------------------

Yeah that is the right place for a new config. It is worth discussing the name as part of the review since this ends up being kind of part of our "api" to the operator.

I would just skip storing the metadata for now (i.e. just throw it away). If we make the change in zk we need a script to grandfather from the old format to the new format. Since we will need a conversion script when we move off zk anyway it makes sense to avoid two conversions for users (one to add the metadata, and another to move off zk).
                
> Add an API to commit offsets
> ----------------------------
>
>                 Key: KAFKA-657
>                 URL: https://issues.apache.org/jira/browse/KAFKA-657
>             Project: Kafka
>          Issue Type: New Feature
>            Reporter: Jay Kreps
>              Labels: project
>         Attachments: KAFKA-657v1.patch, KAFKA-657v2.patch, KAFKA-657v3.patch, KAFKA-657v4.patch, KAFKA-657v5.patch, KAFKA-657v6.patch, KAFKA-657v7.patch
>
>
> Currently the consumer directly writes their offsets to zookeeper. Two problems with this: (1) This is a poor use of zookeeper, and we need to replace it with a more scalable offset store, and (2) it makes it hard to carry over to clients in other languages. A first step towards accomplishing that is to add a proper Kafka API for committing offsets. The initial version of this would just write to zookeeper as we do today, but in the future we would then have the option of changing this.
> This api likely needs to take a sequence of consumer-group/topic/partition/offset entries and commit them all.
> It would be good to do a wiki design on how this would work and consensus on that first.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

 

    [ https://issues.apache.org/jira/browse/KAFKA-657?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13543227#comment-13543227 ]

David Arthur commented on KAFKA-657:
------------------------------------

We had talked about limiting the size of the metadata to 1k or something and making this configurable. My question is: where does this config belong, and how to get it into KafkaApis? Right now, I've added this config to KafkaConfig and have passed in the "config" variable from KafkaServer to KafkaApis.

Also, for the offset payloads, I'm opting to store them as a JSON object in ZooKeeper: {"offset": 42, "metadata": "foo"}
                
> Add an API to commit offsets
> ----------------------------
>
>                 Key: KAFKA-657
>                 URL: https://issues.apache.org/jira/browse/KAFKA-657
>             Project: Kafka
>          Issue Type: New Feature
>            Reporter: Jay Kreps
>              Labels: project
>         Attachments: KAFKA-657v1.patch, KAFKA-657v2.patch, KAFKA-657v3.patch, KAFKA-657v4.patch, KAFKA-657v5.patch, KAFKA-657v6.patch, KAFKA-657v7.patch
>
>
> Currently the consumer directly writes their offsets to zookeeper. Two problems with this: (1) This is a poor use of zookeeper, and we need to replace it with a more scalable offset store, and (2) it makes it hard to carry over to clients in other languages. A first step towards accomplishing that is to add a proper Kafka API for committing offsets. The initial version of this would just write to zookeeper as we do today, but in the future we would then have the option of changing this.
> This api likely needs to take a sequence of consumer-group/topic/partition/offset entries and commit them all.
> It would be good to do a wiki design on how this would work and consensus on that first.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

 

    [ https://issues.apache.org/jira/browse/KAFKA-657?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13543459#comment-13543459 ]

Jay Kreps commented on KAFKA-657:
---------------------------------

Works for me.
                
> Add an API to commit offsets
> ----------------------------
>
>                 Key: KAFKA-657
>                 URL: https://issues.apache.org/jira/browse/KAFKA-657
>             Project: Kafka
>          Issue Type: New Feature
>            Reporter: Jay Kreps
>              Labels: project
>         Attachments: KAFKA-657v1.patch, KAFKA-657v2.patch, KAFKA-657v3.patch, KAFKA-657v4.patch, KAFKA-657v5.patch, KAFKA-657v6.patch, KAFKA-657v7.patch
>
>
> Currently the consumer directly writes their offsets to zookeeper. Two problems with this: (1) This is a poor use of zookeeper, and we need to replace it with a more scalable offset store, and (2) it makes it hard to carry over to clients in other languages. A first step towards accomplishing that is to add a proper Kafka API for committing offsets. The initial version of this would just write to zookeeper as we do today, but in the future we would then have the option of changing this.
> This api likely needs to take a sequence of consumer-group/topic/partition/offset entries and commit them all.
> It would be good to do a wiki design on how this would work and consensus on that first.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

 

    [ https://issues.apache.org/jira/browse/KAFKA-657?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13544054#comment-13544054 ]

Jay Kreps commented on KAFKA-657:
---------------------------------

This looks good to me. If no further objections I am taking this on trunk.
                
> Add an API to commit offsets
> ----------------------------
>
>                 Key: KAFKA-657
>                 URL: https://issues.apache.org/jira/browse/KAFKA-657
>             Project: Kafka
>          Issue Type: New Feature
>            Reporter: Jay Kreps
>              Labels: project
>         Attachments: KAFKA-657v1.patch, KAFKA-657v2.patch, KAFKA-657v3.patch, KAFKA-657v4.patch, KAFKA-657v5.patch, KAFKA-657v6.patch, KAFKA-657v7.patch, KAFKA-657v8.patch
>
>
> Currently the consumer directly writes their offsets to zookeeper. Two problems with this: (1) This is a poor use of zookeeper, and we need to replace it with a more scalable offset store, and (2) it makes it hard to carry over to clients in other languages. A first step towards accomplishing that is to add a proper Kafka API for committing offsets. The initial version of this would just write to zookeeper as we do today, but in the future we would then have the option of changing this.
> This api likely needs to take a sequence of consumer-group/topic/partition/offset entries and commit them all.
> It would be good to do a wiki design on how this would work and consensus on that first.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

 

    [ https://issues.apache.org/jira/browse/KAFKA-657?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13544370#comment-13544370 ]

Jay Kreps commented on KAFKA-657:
---------------------------------

I updated the property name. Normally I am against commenting out code since it is in version control anyway, but in this case those tests are actually useful and not in version control so probably makes sense to leave them.
                
> Add an API to commit offsets
> ----------------------------
>
>                 Key: KAFKA-657
>                 URL: https://issues.apache.org/jira/browse/KAFKA-657
>             Project: Kafka
>          Issue Type: New Feature
>            Reporter: Jay Kreps
>              Labels: project
>             Fix For: 0.8.1
>
>         Attachments: KAFKA-657v1.patch, KAFKA-657v2.patch, KAFKA-657v3.patch, KAFKA-657v4.patch, KAFKA-657v5.patch, KAFKA-657v6.patch, KAFKA-657v7.patch, KAFKA-657v8.patch
>
>
> Currently the consumer directly writes their offsets to zookeeper. Two problems with this: (1) This is a poor use of zookeeper, and we need to replace it with a more scalable offset store, and (2) it makes it hard to carry over to clients in other languages. A first step towards accomplishing that is to add a proper Kafka API for committing offsets. The initial version of this would just write to zookeeper as we do today, but in the future we would then have the option of changing this.
> This api likely needs to take a sequence of consumer-group/topic/partition/offset entries and commit them all.
> It would be good to do a wiki design on how this would work and consensus on that first.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

 

    [ https://issues.apache.org/jira/browse/KAFKA-657?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13544261#comment-13544261 ]

Neha Narkhede commented on KAFKA-657:
-------------------------------------

+1 on v8
                
> Add an API to commit offsets
> ----------------------------
>
>                 Key: KAFKA-657
>                 URL: https://issues.apache.org/jira/browse/KAFKA-657
>             Project: Kafka
>          Issue Type: New Feature
>            Reporter: Jay Kreps
>              Labels: project
>         Attachments: KAFKA-657v1.patch, KAFKA-657v2.patch, KAFKA-657v3.patch, KAFKA-657v4.patch, KAFKA-657v5.patch, KAFKA-657v6.patch, KAFKA-657v7.patch, KAFKA-657v8.patch
>
>
> Currently the consumer directly writes their offsets to zookeeper. Two problems with this: (1) This is a poor use of zookeeper, and we need to replace it with a more scalable offset store, and (2) it makes it hard to carry over to clients in other languages. A first step towards accomplishing that is to add a proper Kafka API for committing offsets. The initial version of this would just write to zookeeper as we do today, but in the future we would then have the option of changing this.
> This api likely needs to take a sequence of consumer-group/topic/partition/offset entries and commit them all.
> It would be good to do a wiki design on how this would work and consensus on that first.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

 

    [ https://issues.apache.org/jira/browse/KAFKA-657?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13542390#comment-13542390 ]

Joel Koshy commented on KAFKA-657:
----------------------------------

Hey David, the patch (and the wiki) looks great.

- For error handling. I think what Jun was referring to is the giant catch clause in handle - i.e., the new keys
  should be added as a case. That junk block of code really needs to be cleaned up :)
- KafkaApis: if(offsetStr == null) : I don't think this can happen.
- Default client id should probably be "" in all the request/responses i.e., to follow convention.
- It would be better to use 1.toShort or val CurrentVersion: Short = 1 (instead of 1.shortValue); although it's more
  or less a non-issue as it's in the object.

                
> Add an API to commit offsets
> ----------------------------
>
>                 Key: KAFKA-657
>                 URL: https://issues.apache.org/jira/browse/KAFKA-657
>             Project: Kafka
>          Issue Type: New Feature
>            Reporter: Jay Kreps
>              Labels: project
>         Attachments: KAFKA-657v1.patch, KAFKA-657v2.patch, KAFKA-657v3.patch, KAFKA-657v4.patch, KAFKA-657v5.patch, KAFKA-657v6.patch
>
>
> Currently the consumer directly writes their offsets to zookeeper. Two problems with this: (1) This is a poor use of zookeeper, and we need to replace it with a more scalable offset store, and (2) it makes it hard to carry over to clients in other languages. A first step towards accomplishing that is to add a proper Kafka API for committing offsets. The initial version of this would just write to zookeeper as we do today, but in the future we would then have the option of changing this.
> This api likely needs to take a sequence of consumer-group/topic/partition/offset entries and commit them all.
> It would be good to do a wiki design on how this would work and consensus on that first.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

 

    [ https://issues.apache.org/jira/browse/KAFKA-657?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13542566#comment-13542566 ]

Jay Kreps commented on KAFKA-657:
---------------------------------

Looks good to me. A few things in addition to the other comments:
76. We baselined the other request versions to 0, but this one is starting at 1. Can we change it to 0 for consistency?
77. It would be good to add the metadata string field.
                
> Add an API to commit offsets
> ----------------------------
>
>                 Key: KAFKA-657
>                 URL: https://issues.apache.org/jira/browse/KAFKA-657
>             Project: Kafka
>          Issue Type: New Feature
>            Reporter: Jay Kreps
>              Labels: project
>         Attachments: KAFKA-657v1.patch, KAFKA-657v2.patch, KAFKA-657v3.patch, KAFKA-657v4.patch, KAFKA-657v5.patch, KAFKA-657v6.patch, KAFKA-657v7.patch
>
>
> Currently the consumer directly writes their offsets to zookeeper. Two problems with this: (1) This is a poor use of zookeeper, and we need to replace it with a more scalable offset store, and (2) it makes it hard to carry over to clients in other languages. A first step towards accomplishing that is to add a proper Kafka API for committing offsets. The initial version of this would just write to zookeeper as we do today, but in the future we would then have the option of changing this.
> This api likely needs to take a sequence of consumer-group/topic/partition/offset entries and commit them all.
> It would be good to do a wiki design on how this would work and consensus on that first.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

 

    [ https://issues.apache.org/jira/browse/KAFKA-657?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13537062#comment-13537062 ]

David Arthur commented on KAFKA-657:
------------------------------------

Jay, the one thing I'm still unclear on are the various failure scenarios. Could you double check that bit of the patch (in KafkaApis.scala)
                
> Add an API to commit offsets
> ----------------------------
>
>                 Key: KAFKA-657
>                 URL: https://issues.apache.org/jira/browse/KAFKA-657
>             Project: Kafka
>          Issue Type: New Feature
>            Reporter: Jay Kreps
>              Labels: project
>         Attachments: KAFKA-657v1.patch, KAFKA-657v2.patch, KAFKA-657v3.patch
>
>
> Currently the consumer directly writes their offsets to zookeeper. Two problems with this: (1) This is a poor use of zookeeper, and we need to replace it with a more scalable offset store, and (2) it makes it hard to carry over to clients in other languages. A first step towards accomplishing that is to add a proper Kafka API for committing offsets. The initial version of this would just write to zookeeper as we do today, but in the future we would then have the option of changing this.
> This api likely needs to take a sequence of consumer-group/topic/partition/offset entries and commit them all.
> It would be good to do a wiki design on how this would work and consensus on that first.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

 

    [ https://issues.apache.org/jira/browse/KAFKA-657?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13537390#comment-13537390 ]

Jay Kreps commented on KAFKA-657:
---------------------------------

Also I had one more generalization I would like to add--metadata field. Sent proposal to the dev list. If no objections I will update the wiki.
                
> Add an API to commit offsets
> ----------------------------
>
>                 Key: KAFKA-657
>                 URL: https://issues.apache.org/jira/browse/KAFKA-657
>             Project: Kafka
>          Issue Type: New Feature
>            Reporter: Jay Kreps
>              Labels: project
>         Attachments: KAFKA-657v1.patch, KAFKA-657v2.patch, KAFKA-657v3.patch, KAFKA-657v4.patch
>
>
> Currently the consumer directly writes their offsets to zookeeper. Two problems with this: (1) This is a poor use of zookeeper, and we need to replace it with a more scalable offset store, and (2) it makes it hard to carry over to clients in other languages. A first step towards accomplishing that is to add a proper Kafka API for committing offsets. The initial version of this would just write to zookeeper as we do today, but in the future we would then have the option of changing this.
> This api likely needs to take a sequence of consumer-group/topic/partition/offset entries and commit them all.
> It would be good to do a wiki design on how this would work and consensus on that first.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

 

    [ https://issues.apache.org/jira/browse/KAFKA-657?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13539141#comment-13539141 ]

Neha Narkhede commented on KAFKA-657:
-------------------------------------

+1 on v5. Just a minor comment which I think Jun raised as well -

KafkaApis
Today, we have a pretty awkward way of handling error codes for the various APIs in the handle() method. We should probably fix that, but until then, it will be good to maintain consistency. The new APIs are missing that error handling.
                
> Add an API to commit offsets
> ----------------------------
>
>                 Key: KAFKA-657
>                 URL: https://issues.apache.org/jira/browse/KAFKA-657
>             Project: Kafka
>          Issue Type: New Feature
>            Reporter: Jay Kreps
>              Labels: project
>         Attachments: KAFKA-657v1.patch, KAFKA-657v2.patch, KAFKA-657v3.patch, KAFKA-657v4.patch, KAFKA-657v5.patch
>
>
> Currently the consumer directly writes their offsets to zookeeper. Two problems with this: (1) This is a poor use of zookeeper, and we need to replace it with a more scalable offset store, and (2) it makes it hard to carry over to clients in other languages. A first step towards accomplishing that is to add a proper Kafka API for committing offsets. The initial version of this would just write to zookeeper as we do today, but in the future we would then have the option of changing this.
> This api likely needs to take a sequence of consumer-group/topic/partition/offset entries and commit them all.
> It would be good to do a wiki design on how this would work and consensus on that first.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira