I feel enforcing users to create OffsetMetadata upon commit brings some overhead, especially in practice most people do not want to embed any metadata with their commits. I would like to propose an alternative solution regarding the API:

commit(Map<TopicPartition, Long>, ComitType, String /* Commit Message /)
commit(Map<TopicPartition, Long>, ComitType, String / Commit Message */, ConsumerCommitCallBack)

And the commit message will be used as the metadata for all the partitions included. It is based on my assumption that users usually would have a single commit message (i.e. the metadata string) per each commit call; if they want to have different messages for different partitions (for example in CopyCat @ewencp), they can call commit() multiple times with the metadata strings on each partition. This of course may cause more round trips for all sync-commit, but I would suggest people to use the following pattern:

commit(async);
commit(async);
...
commit(sync); // last call

Since now the commit calls are all ordered.

@guozhangwang Yeah I agree that it's annoying for most users who don't care about offset metadata. Would the expectation in your suggested API be that the commit message would be tied to the metadata for each topic partition in the map?

@guozhangwang when you say overhead, do you mean from an API usability perspective or from a performance perspective?

Hmm, not sure I like that API. Having a map for the offsets and then requiring all TPs to use the same metadata is odd. And it seems pretty annoying if you want to do async and get a callback when the commit completes -- now you have to manage waiting on all the callbacks to complete as well. Also, splitting the commit across a bunch of separate requests isn't ideal.

Another alternative is to add this variant:

public void commit(Map<TopicPartition, OffsetAndMetadata> offsets, CommitType commitType, ConsumerCommitCallback callback);

but leave the existing Map<TopicPartition, Long> versions. If you want to include metadata, you must provide all the other parameters, which avoids introducing 2x the number of APIs and is reasonable since using metadata is an advanced feature. This has the same impact as the String commitMessage variant since I assume that would be an additional method since most users do not want it. So both would add 1 method, but this version does everything in one round trip, only has one callback, etc.

Copycat actually doesn't have a use case for the metadata currently, so I unfortunately don't have a good example use case for any of this currently.

@ewencp To be clear, that would mean we couldn't provide the same version of that method which uses a Long in the Map, right? As a user, I would probably rather have all the commit methods be consistent, but it may be a reasonable compromise.

I was thinking that the user would maintain Map<TopicPartition, OffsetMetadata>, so map.values() would return Collection<OffsetMetadata>, which could be passed directly to commit. That said, I don't have a strong preference on any of the choices so far, but I'm probably leaning toward the initial approach. The overhead is not so high and users who want to manage their own commits may find metadata more useful than we're imagining. Off the top of my head, I can imagine wanting to add timestamps, hostnames, or maybe versioning information to the commit. If instead it's really such an extreme edge case that almost no users would find value in, maybe it doesn't belong in the API at all and we can think about another way to expose it.

@ewencp Regarding making OffsetAndMetadata mutable, does that make sense if we do defensive copies of the passed in map as is being suggested? It seems to me that ownership rules would be inconsistent in that case and we would be doing plenty of allocation with the map copy anyway. It's also worth saying that due to boxing, we were already allocating Long instances, so the number of allocations doesn't change, it's just the size of each instance.

OK, now I think my proposed approach does not suits well as well after reading through the conversations. If we agree that the percentage of users calling commit with manual offset values is really low, then I think it may be OK for adding this API usability overhead for them with a probably even lower percentage usage. So let's just proceed with the original API.

So let's just proceed with the original API.
@guozhangwang Can you clarify?

@nehanarkhede Yup I can review and check it in once it is rebased.

How about "metadata == null ? that.metadata == null : metadata.equals(that.metadata)"?