KAFKA-13850: Show missing record type in MetadataShell

[dengziming]

More detailed description of your change
Currently, 8 record types are missing in MetadataShell:

1. AccessControlEntryRecord and RemoveAccessControlEntryRecord are added in KIP-801, FeatureLevelRecord was added in KIP-778, and BrokerRegistrationChangeRecord was added in KIP-841, and NoOpRecord was added in KIP-835, I added these 5 record types in MetadataShell.
2. AccessControlRecord will not be used since we use AccessControlEntryRecord.
3. UserScramCredentialRecord, DelegationTokenRecord and will be used in the future, but we can't make sure whether their schema will be changed, just like AccessControlRecord which is outdated. so I didn't add them here.

Summary of testing strategy (including rationale)
Unit tests

Committer Checklist (excluded from commit message)

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

[dengziming]

Just as KIP-778 described, each ACL is shown in /acl/id/ in its JSON form. however, I should mention here that their schema in ZkNode is more hierarchical:

get /kafka/kafka-acl/${resourceType}/${resourceName}
 
{
    "version":1,
    "acls":[
        {
            "principal":"User:kafka",
            "permissionType":"Allow",
            "operation":"Read",
            "host":"1.1.1.1"
        }
    ]
}

[dengziming]

ping @mumrah

[showuon]

@jsancio , I think we should make this fix into v3.3, otherwise, the metadata shell is basically broken. WDYT?
@dengziming , could you help confirm if this patch contain all record types we have now? Thanks.

[showuon]

@dengziming , thanks for the fix, left some comments.

[showuon]

nit: additional empty line

[showuon]

nit: since this is just for test, we can use Uuid.ZERO_UUID for testing, to avoid random strings here.

[dengziming]

This is a little overkill here, we are using random strings in a lot of test cases and it won't reduce the code quality, and Uuid.ZERO_UUID may have some special meaning in different cases.

[showuon]

Sorry, could you let me know where we mentioned featureLevel == 0 means delete it? I checked the record schema, it didn't mention this:

kafka/metadata/src/main/resources/common/metadata/FeatureLevelRecord.json

Lines 17 to 26 in 4b310d1

	"apiKey": 12,
	"type": "metadata",
	"name": "FeatureLevelRecord",
	"validVersions": "0",
	"flexibleVersions": "0+",
	"fields": [
	{ "name": "Name", "type": "string", "versions": "0+",
	"about": "The feature name." },
	{ "name": "FeatureLevel", "type": "int16", "versions": "0+",
	"about": "The current finalized feature level of this feature for the cluster." }

[dengziming]

Firstly we have a RemoveFeatureLevelRecord.json but we removed it in #12207 and we use 0-0 to represent not supported:

kafka/metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java

Line 290 in 4b310d1

if (record.featureLevel() == 0) {

[showuon]

Thanks for sharing! What a magic 0, haha! Could we add a simple note into FeatureLevelRecord.json? Something like:

{ "name": "FeatureLevel", "type": "int16", "versions": "0+", 
     "about": "The current finalized feature level of this feature for the cluster. 0 means feature not enabled." } 

[dengziming]

Good suggestion.

[dengziming]

Thank you @showuon for reviewing, I will fix them soon, I have checked all the record types and their fields, except some records we don't support now, for example, USER_SCRAM_CREDENTIAL_RECORD

[dengziming]

Firstly we have a RemoveFeatureLevelRecord.json but we removed it in #12207 and we use 0-0 to represent not supported:

kafka/metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java

Line 290 in 4b310d1

if (record.featureLevel() == 0) {

[showuon]

Could we not delete this file in this PR? I think it'd be better we raise another JIRA or PR to delete it. I'm not 100% sure we will use it or not. Let's focus on missing record type in this PR. WDYT?

[dengziming]

Yeah, this makes sense, it's reasonable to keep it.

[showuon]

LGTM! Please revert the AccessControlRecord.json change. Thanks.

[showuon]

Thank you! Let's wait for the jenkins build. @jsancio , I'm going to backport this patch to 3.3 tomorrow if you have no objection. Thanks.

[showuon]

Failed tests are un-related:

Build / JDK 11 and Scala 2.13 / kafka.server.KRaftClusterTest.testCreateClusterAndPerformReassignment()

[showuon]

backport to 3.3

[mumrah]

Sorry for the delay @dengziming, yes this is needed for 3.3. Thanks!