We should return TOPIC_AUTHORIZATION_FAILED?

Good catch, fixed

Should we keep this timeout since the default timeout is 0 and could fail a request prematurely?

Good point. Let's set the default timeout to something like a minute, so that we don't get bogus failures in unit tests. (This shouldn't matter for AdminClient since AC always explicitly sets the timeout.)

Should we keep the 1000 timeout?

See above, let's set it in CreateTopicsRequest.json

Should we keep the 1000 timeout?

See above

Should we keep the 10000 timeout?

See above

Should the partition index start at 0?

Fixed

This doesn't really work as a general pattern. What I mean is that for ElectPreferredReplicas there is no analog of NewTopic: The AdminClient API is just a Collection<TopicPartition>, so there's no handy class in which to put the conversion code. It might be worth having a consistent place to put these conversion functions (for AdminClient protocol messages, at least); either a single common class (AdminClientProtocolUtil), or a ElectPreferredReplicasProtocolUtil etc. The latter would have one benefit: With the use of nested classes there's the possibility for imports like import org.apache.kafka.common.message.ElectPreferredLeadersResponseData.Result; to collide when another protocol uses a nested class named Result, and using outer class qualified type names makes the code ugly. Having a ElectPreferredReplicasProtocolUtil etc would avoid this.

I agree that you can't use exactly the same pattern for TopicPartition, since it's a more generic class, not an AdminClient class at all. Probably the best place to put the conversion function is ElectPreferredLeaderElectionRequest.java, since it's relevant to that request.

As a side note, with TopicPartition, the conversion code is pretty simple, right? Something like partitions.map(p -> new ElectPreferredLeadersResponseData.Result.setIndex(p.partition()).setTopic(p.topic()) So you could consider doing it inlline, at least in theory (I haven't looked at the code to see if this makes sense).

NewTopic was more involved since there are multiple options like whether we explicitly choose a partition layout, or just a number of replicas, etc.

Since results is a set, is that check still needed?

Just to be clear, it's a multiset. This was done specifically to preserve the existing behavior where duplicate entries caused us to return an error code, rather than triggering a deserialization error (which would show up as a disconnection)

Is there a coding standard to structure the L815-816? I wonder if we could merge both.

The reason for having two lines is to avoid a line which is too long.

Could we merge L213-214?

ok