dajac commented on code in PR #18127:
URL: https://github.com/apache/kafka/pull/18127#discussion_r1879482075
##########
generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java:
##########
@@ -1343,6 +1343,11 @@ private void generateVariableLengthFieldSize(FieldSpec
field,
buffer.printf("int _sizeBeforeStruct =
_size.totalSize();%n", field.camelCaseName());
buffer.printf("this.%s.addSize(_size, _cache,
_version);%n", field.camelCaseName());
buffer.printf("int _structSize = _size.totalSize() -
_sizeBeforeStruct;%n", field.camelCaseName());
+
VersionConditional.forVersions(field.nullableVersions(), possibleVersions).
+ ifMember(__ -> {
+ buffer.printf("_structSize += 1;%n");
+ }).
+ generate(buffer);
Review Comment:
Don't we need to also change the code at L1336? I think that this is where
the mistake is because we add one byte in all cases for nullable structs. It
should be moved within the tagged/not-tagged branches in order to be correct.
You did it for the tagged case. I suppose that we need to do the same for the
not-tagged case.
I also wonder whether we should do it as follow:
```
buffer.printf("int _sizeBeforeStruct = _size.totalSize();%n",
field.camelCaseName());
VersionConditional.forVersions(field.nullableVersions(), possibleVersions).
ifMember(__ -> {
buffer.printf("_size.addBytes(1);%n");
}).
generate(buffer);
buffer.printf("this.%s.addSize(_size, _cache, _version);%n",
field.camelCaseName());
buffer.printf("int _structSize = _size.totalSize() - _sizeBeforeStruct;%n",
field.camelCaseName());
buffer.printf("_size.addBytes(ByteUtils.sizeOfUnsignedVarint(_structSize));%n");
```
What do you think?
##########
clients/src/test/resources/common/message/StructMessage.json:
##########
@@ -13,7 +13,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.
{
- "name": "NullableStructMessage",
+ "name": "StructMessage",
Review Comment:
Is this change needed? We do have other messages which contains non nullable
structs. Could we extend those?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
