feat(admin): add KIP-396 list/alter offsets APIs

[DCjanus]

Summary

• Add Admin APIs for KIP-396 ListOffsets and AlterConsumerGroupOffsets to support bulk offset queries and group offset commits.
• Provide Go types aligned with KIP-396 semantics (OffsetAndMetadata).

Key changes

• Add ListOffsets and AlterConsumerGroupOffsets to ClusterAdmin and wire protocol requests.
• Add result structs with leader epoch support and commit metadata for admin offset commits.
• Implement broker fan-out for list offsets and coordinator commit path for alter offsets.
• Add functional tests covering timestamp list offsets and admin offset commits.

Constraints / tradeoffs

• ListOffsets currently accepts int64 queries (earliest/latest/timestamp) to stay consistent with existing offset conventions; we can introduce an explicit spec type in a follow-up if needed.

Notes

• Spec reference: KIP-396
• Closes #1669

[puellanivis]

I’m not sure why we need a ton of constructors, when we can simply export this value.

After all, given the functionality provided, we can already arbitrarily mutate any given OffsetSpec and access the field at will:

offspec := OffsetSpecLatest()
offspec = OffsetSpecForTimestamp(arbitraryTimestamp)
go func() {
	offspec = OffsetSpecForTimestamp(OffsetNewest)
}()
ts := offspec.Timestamp() // write-read race condition

[DCjanus]

I’m not sure why we need a ton of constructors, when we can simply export this value.

After all, given the functionality provided, we can already arbitrarily mutate any given OffsetSpec and access the field at will:

offspec := OffsetSpecLatest()
offspec = OffsetSpecForTimestamp(arbitraryTimestamp)
go func() {
	offspec = OffsetSpecForTimestamp(OffsetNewest)
}()
ts := offspec.Timestamp() // write-read race condition

Agree that we should not mix two different input styles for the same concept (as noted in #3419 (comment)).

To stay consistent with existing Sarama APIs (e.g. Client.GetOffset(topic, partition, time int64) which uses OffsetOldest/OffsetNewest), I removed OffsetSpec and made ListOffsets take int64 directly (pass OffsetOldest/OffsetNewest or a millisecond timestamp).

Done in commit f3e40c7.

[puellanivis]

var errs []error
for res := range results {
	if res.err != nil {
		errs = append(errs, res.err)
	}
	...
}

return allResults errors.Join(errs...)

[DCjanus]

var errs []error
for res := range results {
	if res.err != nil {
		errs = append(errs, res.err)
	}
	...
}

return allResults errors.Join(errs...)

Addressed in commit bce2339 by aggregating all errors with errors.Join.

[puellanivis]

How many requests are we likely to be spinning off here?

It can happen that spinning off too many goroutines will actually be performance-degrading rather than performance-enhancing.

[DCjanus]

How many requests are we likely to be spinning off here?

It can happen that spinning off too many goroutines will actually be performance-degrading rather than performance-enhancing.

Thanks for the note. The Java AdminClient doesn’t appear to impose an explicit concurrency cap either — it groups by broker and issues one in‑flight request per broker via the admin I/O loop.

Key entry point (no concurrency limiting logic):

• KafkaAdminClient#listOffsets

In practice, the Java I/O loop is equivalent to spawning goroutines in Go: both drive concurrent in‑flight requests without a per‑broker cap.

Given typical Kafka clusters are well below 1,000 brokers, I believe even 10,000 concurrent in‑flight requests should be relatively easy for modern hardware to handle.

[puellanivis]

Our current design uses map[string]map[int32]V adding this TopicPartitionID might have been nice if we had started with it, but we haven’t really. https://pkg.go.dev/github.com/IBM/sarama#AlterPartitionReassignmentsResponse

Providing two different ways to do something is likely to increase confusion over any benefits of flattening the maps.

It also reduces the ability to iterate over topics individually, without needing to then iterate over all Topic × Partition combinations and select for Topic.

[DCjanus]

Our current design uses map[string]map[int32]V adding this TopicPartitionID might have been nice if we had started with it, but we haven’t really. https://pkg.go.dev/github.com/IBM/sarama#AlterPartitionReassignmentsResponse

Providing two different ways to do something is likely to increase confusion over any benefits of flattening the maps.

It also reduces the ability to iterate over topics individually, without needing to then iterate over all Topic × Partition combinations and select for Topic.

Thanks for the point about keeping the API shape consistent. I reverted to the existing map[string]map[int32]V style so callers can iterate by topic without scanning all partitions.

This removes TopicPartitionID from ListOffsets and aligns the input/return maps with the rest of Sarama’s admin APIs.

Done in commit be65797.

[puellanivis]

I’m not sure this is an Offsets result? As it seems to be single offset result?

[DCjanus]

Renamed ListOffsetsResult to OffsetResult to reflect the single-partition result; done in commit f9150fe.

[puellanivis]

We could also isolate/scope these types into the ListOffsets right?

[DCjanus]

We could also isolate/scope these types into the ListOffsets right?

Updated in commit 243445e (scoped the helper types inside ListOffsets).

[puellanivis]

I’m not sure why we double track the broker? We’re using it as the key, and as a field in the value?

We could just for broker, req := range requests { … } below, right?

[DCjanus]

I’m not sure why we double track the broker? We’re using it as the key, and as a field in the value?

We could just for broker, req := range requests { … } below, right?

Addressed in commit ea32b6e by removing the redundant broker field and passing the broker via the map key in the loop.

[puellanivis]

Looks pretty good, just two things that are perhaps more style than substance.

[puellanivis]

[nitpick] We’re compiling this under go.mod go version v1.24.0, which should be well enough into the change of semantics for loopvars that removed the need to passing these variables through arguments, rather than accessing them by simple closure. 🤔 Though, I’m not sure how we would want to approach changing this common pattern, even if it is now unnecessary.

[DCjanus]

[nitpick] We’re compiling this under go.mod go version v1.24.0, which should be well enough into the change of semantics for loopvars that removed the need to passing these variables through arguments, rather than accessing them by simple closure. 🤔 Though, I’m not sure how we would want to approach changing this common pattern, even if it is now unnecessary.

Thanks for the style note.

Personally, I prefer passing loop variables into goroutines so it is clear what is shared.

I also want to match the project style. If you think it is better to rely on the Go 1.24 behavior, I can change it — or you can push a commit since maintainer edits are enabled.

[puellanivis]

The usual problem is that to know for sure which values are being put into these parameters, one has to drop all the way to the bottom of the go func() { … }() to check, (though good here, you’re also shadowing the originals, so one cannot accidentally access the wrong one; since whether one likes to be explicit or not, the implicit closure access is always there), and it requires repeating the type information of the parameters as well, while when accessed through closure.

So, if we refactor a type name of one of the variables, this is then Yet Another Place where the type-name must be refactored.

PS: To be more clear, because I forgot to mention it, this is fine. 🤷‍♀️

[DCjanus]

The usual problem is that to know for sure which values are being put into these parameters, one has to drop all the way to the bottom of the go func() { … }() to check, (though good here, you’re also shadowing the originals, so one cannot accidentally access the wrong one; since whether one likes to be explicit or not, the implicit closure access is always there), and it requires repeating the type information of the parameters as well, while when accessed through closure.

So, if we refactor a type name of one of the variables, this is then Yet Another Place where the type-name must be refactored.

PS: To be more clear, because I forgot to mention it, this is fine. 🤷‍♀️

Thanks for the clarification — you convinced me. I switched this to simple closure capture in 3d11625.

In a language without a “no implicit capture” guarantee, explicit parameters don’t really reduce review burden, since you still have to check for other implicit captures.

[puellanivis]

As a seeming reversal of my prior guidance; since this type is function internal, we could also use the topicPartition type as a key here, and avoid needing more than the one map.

(One of those cases where blackbox internals can choose different paradigms from the public API.)

[DCjanus]

As a seeming reversal of my prior guidance; since this type is function internal, we could also use the topicPartition type as a key here, and avoid needing more than the one map.

(One of those cases where blackbox internals can choose different paradigms from the public API.)

Thanks! Updated in 494f7c3.

[dnwe]

I did a quick first pass, one stylistic comment below, but also one main question here: is there a reason why we have the ca.retryOnError wrapper on AlterConsumerGroupOffsets, but not on any of the calls in ListOffsets?

[dnwe]

Can we structure these in descending order like we do elsewhere in Sarama? e.g.,

sarama/offset_fetch_request.go

Lines 23 to 46 in 2fb8eaf

	if version.IsAtLeast(V2_5_0_0) {
	// Version 7 is adding the require stable flag.
	request.Version = 7
	} else if version.IsAtLeast(V2_4_0_0) {
	// Version 6 is the first flexible version.
	request.Version = 6
	} else if version.IsAtLeast(V2_1_0_0) {
	// Version 3, 4, and 5 are the same as version 2.
	request.Version = 5
	} else if version.IsAtLeast(V2_0_0_0) {
	request.Version = 4
	} else if version.IsAtLeast(V0_11_0_0) {
	request.Version = 3
	} else if version.IsAtLeast(V0_10_2_0) {
	// Starting in version 2, the request can contain a null topics array to indicate that offsets
	// for all topics should be fetched. It also returns a top level error code
	// for group or coordinator level errors.
	request.Version = 2
	} else if version.IsAtLeast(V0_8_2_0) {
	// In version 0, the request read offsets from ZK.
	//
	// Starting in version 1, the broker supports fetching offsets from the internal __consumer_offsets topic.
	request.Version = 1
	}

It is somewhat stylistic (but also avoids multiple assignment). It is also useful to have the version-to-version additions (taken from Kafka's protocol json files) to show what changed between them too

You may also want to move this into offset_commit_request.go as a NewOffsetCommitRequest func that sets up the correct version etc.

[DCjanus]

Can we structure these in descending order like we do elsewhere in Sarama? e.g.,

sarama/offset_fetch_request.go

Lines 23 to 46 in 2fb8eaf

	if version.IsAtLeast(V2_5_0_0) {
	// Version 7 is adding the require stable flag.
	request.Version = 7
	} else if version.IsAtLeast(V2_4_0_0) {
	// Version 6 is the first flexible version.
	request.Version = 6
	} else if version.IsAtLeast(V2_1_0_0) {
	// Version 3, 4, and 5 are the same as version 2.
	request.Version = 5
	} else if version.IsAtLeast(V2_0_0_0) {
	request.Version = 4
	} else if version.IsAtLeast(V0_11_0_0) {
	request.Version = 3
	} else if version.IsAtLeast(V0_10_2_0) {
	// Starting in version 2, the request can contain a null topics array to indicate that offsets
	// for all topics should be fetched. It also returns a top level error code
	// for group or coordinator level errors.
	request.Version = 2
	} else if version.IsAtLeast(V0_8_2_0) {
	// In version 0, the request read offsets from ZK.
	//
	// Starting in version 1, the broker supports fetching offsets from the internal __consumer_offsets topic.
	request.Version = 1
	}

It is somewhat stylistic (but also avoids multiple assignment). It is also useful to have the version-to-version additions (taken from Kafka's protocol json files) to show what changed between them too

You may also want to move this into offset_commit_request.go as a NewOffsetCommitRequest func that sets up the correct version etc.

• Switched to descending version checks with explicit version-diff comments.
• Moved the admin OffsetCommitRequest initializer to offset_commit_request.go as NewOffsetCommitRequest and used it from admin_offsets.go.

Done in commit c525eb0

[puellanivis]

I wanna add as an aside, but we can also switch to a more go-style:

switch {
case version.IsAtLeast(V2_5_0_0):
case version.IsAtLeast(V2_4_0_0):
...
}

But I’ve not mentioned this before, because the style already here is are if … else if … else … cascades.

[DCjanus]

I wanna add as an aside, but we can also switch to a more go-style:

switch {
case version.IsAtLeast(V2_5_0_0):
case version.IsAtLeast(V2_4_0_0):
...
}

But I’ve not mentioned this before, because the style already here is are if … else if … else … cascades.

Thanks for the suggestion! I agree it’s more Go‑style.

To keep this PR focused and clean, I’d prefer not to change this here.

@dnwe if you also prefer the switch {} style, I can follow up with a small PR to convert similar cascades across the project.

[DCjanus]

I did a quick first pass, one stylistic comment below, but also one main question here: is there a reason why we have the ca.retryOnError wrapper on AlterConsumerGroupOffsets, but not on any of the calls in ListOffsets?

I missed the retry logic earlier; added it in commit 4446246.

[DCjanus]

Hi @dnwe,

I saw the new AI-assistance guidance in #3452, including the note about not putting AI tools/models in Co-authored-by.

Some commits in this PR include:Co-authored-by: OpenAI Codex <codex@openai.com>

I add that via a global AGENTS.md rule across repos to keep AI disclosure explicit and consistent. This also makes it easy for maintainers who prefer to avoid AI-assisted contributions to identify them quickly (I may have a different view, but I respect that preference).

I also understand why Signed-off-by should remain human-only, since AI agents do not have legal identity for attestation.

Before opening PRs, I still review and validate all changes myself.

Given the new policy, what would you prefer for this PR?
Should I rewrite history (force-push) to remove those Co-authored-by trailers, or keep existing commits as-is and follow the new rule from now on?

Happy to follow your preference.

(Also, this comment was lightly polished with AI for clarity because English is not my native language, and I want to avoid ambiguity in technical communication.)

[DCjanus]

@dnwe Quick correction: for this PR, I had not enabled my AGENTS.md co-author rule yet, so these commits do not include an AI Co-authored-by trailer.

I still want to discuss the policy in general, but this specific PR is not an example of that case.

[puellanivis]

So, it’s inappropriate to attribute co-authorship to anything that is not a person, and thus not able to hold any copyrights. Having random public domain uncopyrightable code mixed into your PR muddies the protections and/or requirements that may be necessary for license compliance.

[puellanivis]

Ah, errors.AsType is in v1.26 so not available yet for us.