MINOR: add versioning to request and response headers

[cmccabe]

Add a version number to request and response headers. The header version is determined by the first two 16 bit fields read (API key and API version). For now, ControlledShutdown v0 has header version 0, and all other requests have v1. Once KIP-482 is implemented, there will be a v2 of the header which supports tagged fields.

[soondenana]

Looks good, left a few comments/nits.

Looking forward to see the PR where v2 is introduced.

[dajac]

LGTM. Thanks for the PR.

It seems checkstyle does not pass though.

[jsancio]

Thanks for the changes @colinhicks. I have a few questions.

[cmccabe]

rebased on trunk

[cmccabe]

Jenkins is having git errors again ☹️

08:31:06 ERROR: Error cloning remote repo 'origin'
08:31:06 hudson.plugins.git.GitException: Command "git fetch --tags --progress git://github.com/apache/kafka.git +refs/heads/*:refs/remotes/origin/*" returned status code 128:

[mumrah]

LGTM. Thanks Colin, formalizing the header fields as a schema and decoupling them from the protocols is a big improvement. +1

[mumrah]

retest this please

[ijuma]

There's a bit of a change in behavior in this PR. We would previously return an empty string for older request versions and now we return null. Is that intended @cmccabe? cc @hachikuji

[ijuma]

Actually, it's not just older requests, the default was always "" whereas I am seeing null after this PR. It looks like there are two issues:

1. We didn't set a default of "" for clientId in the json schema.
2. The generated protocol code behaves differently with regards to default values. It only uses the default if the field is not present. The Struct code uses it if value == null.

        Object value = this.values[field.index];
        if (value != null)
            return value;
        else if (field.def.hasDefaultValue)
            return field.def.defaultValue;
        else if (field.def.type.isNullable())
            return null;
        else
            throw new SchemaException("Missing value for field '" + field.def.name + "' which has no default value.");

This is the generated code after I add the "" default for clientId.

public void read(Readable readable, short version) {
        this.requestApiKey = readable.readShort();
        this.requestApiVersion = readable.readShort();
        this.correlationId = readable.readInt();
        if (version >= 1) {
            this.clientId = readable.readNullableString();
        } else {
            this.clientId = "";
        }
    }

[cmccabe]

The out-of-the-box default for string fields in the generated schema is always the empty string, never null. null would have to have been specifically chosen, which we did not. So you shouldn't have to specifically choose the empty string as a default.

I was (am?) confused about whether null is a valid clientId. I saw some unit tests that were using that value, but I didn't see anywhere else in the code that was doing it. It's not clear to me what the distinction between a null clientId and an empty one is. If they are just treated interchangeably by the code, maybe what we really need is some code to translate null -> the empty string on the server side. This will help ensure that weird old clients keep working.

[ijuma]

The protocol behavior is that null is valid while we use a default empty string so that it's never seen by users. The generated protocol classes definitely change behavior here as it caused a previously passing test to fail with a NPE. I'll share the details with you offline.