[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14741654#comment-14741654 ] ASF GitHub Bot commented on KAFKA-2389: --- Github user asfgit closed the pull request at: https://github.com/apache/kafka/pull/134 > CommitType seems not necessary in commit(). > --- > > Key: KAFKA-2389 > URL: https://issues.apache.org/jira/browse/KAFKA-2389 > Project: Kafka > Issue Type: Sub-task >Reporter: Jiangjie Qin >Assignee: Jiangjie Qin > Fix For: 0.8.3 > > > The CommitType does not seem to be necessary in for commit(), it can be > inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14715805#comment-14715805 ] Ewen Cheslack-Postava commented on KAFKA-2389: -- I'd also say that just because the Zookeeper API has it doesn't make it good API design... I don't know about other people, but I raised the concern about naming again in the code review because I actually did get confused about what the semantics of each method signature was. CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14715846#comment-14715846 ] Guozhang Wang commented on KAFKA-2389: -- Yeah, I did not think that Zookeeper API is the golden standard and we should just align with it, I guess I am just subjectively leaning towards the same function names for sync / async since for sync methods users can always put their callback logic after the commit() call hence they do not need a callback parameter, and wanted to use Zookeeper API as a backing point. Regarding [~becket_qin]'s point I agree that would be the issue. Anyways since this is pure personal taste I am willing to go with the majority vote. CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14715565#comment-14715565 ] Guozhang Wang commented on KAFKA-2389: -- I'm late on this discussion, but here are my two cents: 1. We need to rename the async commits to commitAsync or something similar, you can't have two methods with the same name that behave totally differently and have different post-conditions. I feel it is OK to have sync / async differentiated by the callback rather than by their names. For example ZooKeeper has a similar approach regarding sync / async APIs: http://zookeeper.apache.org/doc/r3.4.5/api/org/apache/zookeeper/ZooKeeper.html#exists(java.lang.String, org.apache.zookeeper.Watcher, org.apache.zookeeper.AsyncCallback.StatCallback, java.lang.Object) 2. We need to include variants of asyncCommit that don't take the callback. Having the user implement or discover a NoOpCallback to be able to use the api is not good. With commit(OffsetCommitCallback callback), users can just call commit(null) and do not need to implement a NoOpCallback, right? I am personally not favor of making commitSync / commitAsync function names. CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14709609#comment-14709609 ] Jay Kreps commented on KAFKA-2389: -- I think this may be the first time Ewen has ever agreed with me. :-) CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14709620#comment-14709620 ] Jiangjie Qin commented on KAFKA-2389: - I am also thinking about having only no parameters map + callback. Passing in a null map is less common in API but it does make the API cleaner. I'll follow this approach and submit another patch. And also glad Jay and Ewen agreed for the first time :P CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14709631#comment-14709631 ] Jason Gustafson commented on KAFKA-2389: [~ewencp] If I understand correctly, you are proposing to drop commitAsync(callback). Is that right? But I can imagine the user wanting to commit the current offsets (whatever they are), and getting some notification for when it actually happens (and whether it failed). I would probably support all three variants: no parameters, callback, map + callback. CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14708866#comment-14708866 ] Ewen Cheslack-Postava commented on KAFKA-2389: -- If I'm understanding people's current positions, I think I agree with Jay that we should take an all-or-nothing approach for async. People who want async should know what they are doing. If they provide no args, they expect an async commit with the current offsets; if they provide arguments, they might have to provide a bit more information (OffsetAndMetadata rather than just offsets), but that burden is perfectly acceptable for a relatively unusual use case. Anyone using async commit should be comfortable passing in a callback (or null if appropriate) for a callback to an async method. However, I still think the commitAsync naming (or any similar differentiation between sync and async commits) helps to make it clear to the user the semantics of the method they are invoking. So I think 2 methods (no parameters map + callback) for the async variants should work well enough. CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14708480#comment-14708480 ] Jay Kreps commented on KAFKA-2389: -- Why would not passing in a callback imply that you want the call to be synchronous? Not passing in a callback just implies you don't have an action to carry out after the commit is completed. Since we in any case only guarantee at-least once delivery doing commit async with no callback is totally valid (if the commit fails you basically get dups which you could in any case get). I actually think it is helpful to have a visual thing that tells you if the call is sync or not so if we have some problem with the CommitType flag then we can split the method into commitSync(...) and commitAsync(...) the challenge with this is that if there are multiple variants on arguments you have to do them all twice which is annoying for the user to troll through. CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14708553#comment-14708553 ] Jay Kreps commented on KAFKA-2389: -- I agree with you that few people will want the callback in the sync mode, that just means they'll use the variant without it. Using the callback with sync mode is still well defined and has exactly the expected meaning (do this thing after the call is complete) it just isn't super useful because you could just as easily put your code afterwards. So I don't think there is anything confusing about it. I'm okay with your proposal too, but 1. We need to rename the async commits to commitAsync or something similar, you can't have two methods with the same name that behave totally differently and have different post-conditions. 2. We need to include variants of asyncCommit that don't take the callback. Having the user implement or discover a NoOpCallback to be able to use the api is not good. The downside of this approach, as I pointed out, is that it leads to 8 variants of commit. Unfortunately the thing about variants is that in practice the user really does have to read all of them to make sure they're using the one they want. This means we make the common case gets more complicated still (adding callbacks already exploded this a bit). CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[
https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14708541#comment-14708541
]
Jiangjie Qin commented on KAFKA-2389:
-
[~jkreps] I think a synchronous call taking a callback sounds a little bit
weird. If it is synchronous, what is the point of having a callback? It looks
intuitive to me that if the synchronous commit succeeds, the method return
without any error, otherwise it throws exception.
So for commit, I think there are three cases:
1. Synchronous commit - commit(), commit(OffsetMap)
2. Asynchronous commit without a callback - commit(NoOpCallback),
commit(OffsetMap, NoOpCallback)
3. Asynchronous commit with a callback - commit(callback), commit(OffsetMap,
callback).
In this case, we only need the following commit methods:
{code}
// Synchronous commit
void commit();
// Synchronous commit with an offset map
void commit(MapTopicPartition, OffsetAndMetadata offsetMap);
// Asynchronous commit
void commit(OffsetCommitCallback callback);
// Asynchronous commit with an offset map
void commit(MapTopicPartition, OffsetAndMetadata offsetMap,
OffsetCommitCallback callback);
{code}
We can make the name more explicit if we want.
CommitType seems not necessary in commit().
---
Key: KAFKA-2389
URL: https://issues.apache.org/jira/browse/KAFKA-2389
Project: Kafka
Issue Type: Sub-task
Reporter: Jiangjie Qin
Assignee: Jiangjie Qin
The CommitType does not seem to be necessary in for commit(), it can be
inferred from whether user passed in a callback or not.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[
https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14708569#comment-14708569
]
Jiangjie Qin commented on KAFKA-2389:
-
Yeah, having commitAsync() and commitSync would be more clear. Are you
suggesting the following API?
{code}
// Synchronous commit
void commitSync();
// Synchronous commit with an offset map
void commitSync(MapTopicPartition, OffsetAndMetadata offsetMap);
// Asynchronous commit without a callback
void commitAsync();
// Asynchronous commit
void commitAsync(OffsetCommitCallback callback);
// Asynchronous commit with an offset map without a callback
void commitAsync(MapTopicPartition, OffsetAndMetadata offsetMap)
// Asynchronous commit with an offset map
void commitAsync(MapTopicPartition, OffsetAndMetadata offsetMap,
OffsetCommitCallback callback);
{code}
The problem of having commitAsync(OffsetMap) and commitAsync(callback) is that
user can not simply pass in a null because it is ambiguous. Personally I don't
think there is too much difference in user experience between commitAsync(null)
and commitAsync(). So personally I think the following API might look cleaner:
{code}
// Synchronous commit
void commitSync();
// Synchronous commit with an offset map
void commitSync(MapTopicPartition, OffsetAndMetadata offsetMap);
// Asynchronous commit
void commitAsync(OffsetCommitCallback callback);
// Asynchronous commit with an offset map
void commitAsync(MapTopicPartition, OffsetAndMetadata offsetMap,
OffsetCommitCallback callback);
{code}
If user don't want a callback, they can just put null for the callback.
CommitType seems not necessary in commit().
---
Key: KAFKA-2389
URL: https://issues.apache.org/jira/browse/KAFKA-2389
Project: Kafka
Issue Type: Sub-task
Reporter: Jiangjie Qin
Assignee: Jiangjie Qin
The CommitType does not seem to be necessary in for commit(), it can be
inferred from whether user passed in a callback or not.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14708577#comment-14708577 ] Jason Gustafson commented on KAFKA-2389: +1 This suggestion has nice symmetry. Passing null as an argument always leaves a bad taste, but if they are trying to manage commits manually, then they probably should be providing the callback, so this forces them to make a conscious decision not to. CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14708587#comment-14708587 ] Jay Kreps commented on KAFKA-2389: -- I personally think the current API is preferable. I don't mind omitting some of the commitAsync variants. We should definitely not omit the no-arg version, though, commitAsync(), since that will likely be the most common. CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14694519#comment-14694519 ] ASF GitHub Bot commented on KAFKA-2389: --- GitHub user becketqin opened a pull request: https://github.com/apache/kafka/pull/134 KAFKA-2389: remove commit type from new consumer. A shot to remove commit type from new consumer. The coordinator constructor takes a default offset commit callback mainly for testing purpose. You can merge this pull request into a Git repository by running: $ git pull https://github.com/becketqin/kafka KAFKA-2389 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/kafka/pull/134.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #134 commit 5f332efa6c690fad730278283d8c419c6a223a8e Author: Jiangjie Qin becket@gmail.com Date: 2015-08-11T19:41:15Z KAFKA-2389: Remove commit type from commit() CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14662618#comment-14662618 ] Jiangjie Qin commented on KAFKA-2389: - [~gwenshap], we talked with [~ewencp] about this before, my understanding of the reason future does not quite apply here is that KafkaConsumer is single threaded, so it is a little bit weird for a thread to call future.get() and meanwhile have to complete the future by itself. CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14662612#comment-14662612 ] Gwen Shapira commented on KAFKA-2389: - mmm... that actually sounds pretty reasonable to me. Doesn't the same issue exist for the Producer? CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14662543#comment-14662543 ] Jason Gustafson commented on KAFKA-2389: [~becket_qin] I'm definitely in favor of removing CommitType if we can find an intuitive way to express the two behaviors. Are you suggesting above that if the user passes null for the callback then the commit is synchronous? Or would the methods which don't accept a callback be treated as synchronous? CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14662572#comment-14662572 ] Jason Gustafson commented on KAFKA-2389: [~gwenshap] This only reason we've resisted adding a Future is that the KafkaConsumer is not thread-safe, which means that the returned Future would have to be confined to the same thread as the consumer. This might be a surprising/confusing restriction for users. CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-2389) CommitType seems not necessary in commit().
[ https://issues.apache.org/jira/browse/KAFKA-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14662624#comment-14662624 ] Gwen Shapira commented on KAFKA-2389: - ahhh, got it. CommitType seems not necessary in commit(). --- Key: KAFKA-2389 URL: https://issues.apache.org/jira/browse/KAFKA-2389 Project: Kafka Issue Type: Sub-task Reporter: Jiangjie Qin Assignee: Jiangjie Qin The CommitType does not seem to be necessary in for commit(), it can be inferred from whether user passed in a callback or not. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
