KAFKA-10326 Both serializer and deserializer should be able to see th…

[chia7712]

https://issues.apache.org/jira/browse/KAFKA-10326

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[chia7712]

EosBetaUpgradeIntegrationTest is flaky... retest this please

[abbccdda]

Are there any serde use cases that require client id?

[chia7712]

Are there any serde use cases that require client id?

I observed this issue when we are removing custom client id config. It causes error as the metrics of our serde is encoded with client id.

[abbccdda]

The change makes sense to me. Will also let @guozhangwang take a look. Btw, I don't think this is an improvement rather than a bug, as we don't have any guarantee to see client id in serdes before.

[abbccdda]

s/originals/copyWithOverride?

[chia7712]

Will copy that

[chia7712]

I have updated the method name.

[chia7712]

Btw, I don't think this is an improvement rather than a bug, as we don't have any guarantee to see client id in serdes before.

There is another reason. We do pass generated client id to metric reporter. It seems to me all plugins should see consistent props.

[guozhangwang]

This change makes sense to me, thanks @chia7712

One (very) minor thing is that, enableAutoCommit may also be overridden when passing into the coordinator object, i.e. the value read from ConsumerConfig may be different from what's actually used. I'm not too worry that this would be leveraged by serde but just want to bring this up.

[chia7712]

@guozhangwang thanks for reviews

One (very) minor thing is that, may also be overridden when passing into the coordinator object, i.e. the value read from ConsumerConfig may be different from what's actually used. I'm not too worry that this would be leveraged by serde but just want to bring this up.

Let me address it to make consistent configs for all plugins.

[chia7712]

org.apache.kafka.clients.admin.KafkaAdminClientTest.testMetadataRetries is traced by #9091 and EosBetaUpgradeIntegrationTest.shouldUpgradeFromEosAlphaToEosBeta[true] is flaky. retest this please

[abbccdda]

This seems beyond the scope of this PR. Why do we also need to let deserializer see whether given consumer instance is auto commit enabled?

[chia7712]

This flag is included by reason of this comment #9102 (comment)

I’m ok to revert it if it is overkill

[abbccdda]

Sorry for the delay, I suggest we revert this part to properly scope this PR.

[chia7712]

@abbccdda thanks for suggestion. I have reverted it.

[chia7712]

@guozhangwang @abbccdda More comments?

[chia7712]

@guozhangwang @abbccdda ping ~

[guozhangwang]

I took a quick look and it lgtm. @abbccdda If your pass is good please feel free to merge.

[chia7712]

@abbccdda ping~

[chia7712]

rebase to trigger new QA

[guozhangwang]

I've merged it to trunk and marked fix version as 2.7, thanks a lot for your contribution @chia7712 !