KAFKA-18889: Make records in ShareFetchResponse non-nullable

[apoorvmittal10]

This PR marks the records as non-nullable for ShareFetch.

This PR is as per the changes for Fetch:
#18726 and some work for ShareFetch
was done here: #19167. I tested with
marking records as non-nullable in ShareFetch, which required
additional handling. The same has been fixed in current PR.

Reviewers: Andrew Schofield aschofield@confluent.io, Chia-Ping Tsai
chia7712@gmail.com, TengYao Chi frankvicky@apache.org, PoAn Yang
payang@apache.org

[AndrewJSchofield]

Should the records field in the ShareFetchResponse.json be non-nullable too? I notice that it permits nulls.

[apoorvmittal10]

Should the records field in the ShareFetchResponse.json be non-nullable too? I notice that it permits nulls.

So in this PR: https://github.com/apache/kafka/pull/19131/files the field was made nullable again. I aligned the changes as per regular Fetch behaviour.

[AndrewJSchofield]

So in this PR: https://github.com/apache/kafka/pull/19131/files the field was made nullable again. I aligned the changes as per regular Fetch behaviour.

I know there was some back and forth about the regular Fetch because of legacy clients, but I don't think we are impeded by those at all.

[frankvicky]

The general idea from #18726 (comment) is that we will make FetchResponse not return null records through a KIP.
I'm not sure we need a KIP for the shared fetch response since Queue for Kafka is still in development.

referenc: https://issues.apache.org/jira/browse/KAFKA-18934

[apoorvmittal10]

Thanks @frankvicky. @AndrewJSchofield What do you think, should I remove the nullable and then later we update the KIP-932 or leave for future?

[AndrewJSchofield]

Thanks @frankvicky. @AndrewJSchofield What do you think, should I remove the nullable and then later we update the KIP-932 or leave for future?

My view is that we remove the nullable now. It would be worth having a quick check to see what difference is made in the generated code, but really if we do not want null values, we should take that step now so the non-Java clients are written with the right expectations too.

[frankvicky]

Make sense to me.
We also need to update the KIP and notify the original email thread.

[frankvicky]

@apoorvmittal10: Thanks for the patch.
LGTM

[AndrewJSchofield]

Technically, that would be "nullableVersions": "0" I suppose. It was nullable in v0. We are now in v1.

[chia7712]

the v0 is not available for now, so it should be fine to remove nullableVersions?

[AndrewJSchofield]

Well, yes, unless we are looking to have the JSON file document what was previously there too for v0. Otherwise, we could entirely remove all mention of v0, which we did not do.

[apoorvmittal10]

@AndrewJSchofield I added the suggestion, only specific to version 0.

[AndrewJSchofield]

lgtm