MINOR: Simplify ApiKeys by relying on ApiMessageType by ijuma · Pull Request #9748 · apache/kafka

ijuma · 2020-12-14T15:42:17Z

The naming for ListOffsets was inconsistent, in some places it was ListOffset and in others
it was ListOffsets. Picked the latter since it was used in metrics and the protocol documentation
and made it consistent.
Removed unused methods in ApiKeys.
Deleted CommonFields.
Added lowestSupportedVersion and highestSupportedVersion to ApiMessageType
Removed tests in MessageTest that are no longer relevant.

Committer Checklist (excluded from commit message)

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

chia7712

@ijuma Thanks for this nice cleanup.

chia7712 · 2020-12-15T00:55:24Z

It is a bit weird that messageType.name and messageType.name() return different string.

Yeah, that's an existing problem with ApiKeys too. The enum method cannot be overridden. I think it should be tackled separately.

chia7712 · 2020-12-15T01:05:47Z

Could we fix the comment about "name" as it is used to be a part of metrics name (see https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/network/RequestChannel.scala#L239). The origin comment is stale.

Good catch.

chia7712 · 2020-12-15T01:07:25Z

this method is never used.

chia7712

As the "id" is removed from the construction, is it possible to add new ApiKeys instances with incorrect order in the future? the static field ID_TO_TYPE assume all ApiKeys instances are created by id order.

chia7712 · 2020-12-15T01:20:41Z

It seems to me this check should be moved to ApiMessageTypeGenerator as it is a generated code from json.

We can, but it doesn't seem particularly beneficial. It's important to generate code that is specific to each data class. Generic code is best written without generation.

We can move this code to a base class for ApiMessageType potentially, particularly if we decide to remove ApiKeys.

it doesn't seem particularly beneficial

It seems to me the benefit is the error happens from runtime/test_runtime to build time (generate message code).

Oh, I see what you mean. You're suggesting to validate during generation, not to include the validation in the generated code.

I took a look at this and not clear how to validate this in the generator. I added unit tests to ApiMessageTypeTest instead and I think that's good enough for now. What do you think?

That alternative looks good to me :)

chia7712 · 2020-12-15T01:23:49Z

…sed methods

ijuma · 2020-12-15T15:23:59Z

As the "id" is removed from the construction, is it possible to add new ApiKeys instances with incorrect order in the future? the static field ID_TO_TYPE assume all ApiKeys instances are created by id order.

@chia7712 Can you please elaborate? I don't see the assumption you mention. The main assumption is that api keys are dense. I changed the code not to rely on the latter although it doesn't change the behavior.

chia7712 · 2020-12-15T15:37:25Z

Can you please elaborate? I don't see the assumption you mention. The main assumption is that api keys are dense. I changed the code not to rely on the latter although it doesn't change the behavior.

I assumed the order of instancing ApiKeys is strict so removing the id may makes it hard to "see" where to put the new ApiKeys in the future. However, it seems the order of instancing ApiKeys is free so the worry is invalid.

…keys * apache-github/trunk: KAFKA-10776: Add version attribute in RequestsPerSec metrics documentation (apache#9661) KAFKA-10854: fix flaky testConnectionRatePerIp test (apache#9752) KAFKA-10525: Emit JSONs with new auto-generated schema (KIP-673) (apache#9526)

ijuma · 2020-12-15T22:55:25Z

Tests passed for JDK 11 and 15. JDK 8 had one flaky failure:

kafka.api.ConsumerBounceTest.testClose failed

@chia7712 Do you have cycles to review this?

chia7712

@ijuma +1 and thanks for this big cleanup!

chia7712 · 2020-12-16T01:02:07Z

-            idToType[key.id] = key;
-        ID_TO_TYPE = idToType;
-        MAX_API_KEY = maxKey;
+            ID_TO_TYPE.put((int) key.id, key);


Does it need to check duplicate id?

I think the generator checks for this. I will double check.

Yes, the generator checks it:

Caused by: java.lang.RuntimeException: Found more than one response with API key 3

There's also ApiMessageTypeTest.testUniqueness that verifies it after the generation.

I added a comment explaining that uniqueness is ensured.

chia7712 · 2020-12-16T01:15:07Z

@@ -361,69 +172,35 @@ private static boolean shouldRetainsBufferReference(Schema[] requestSchemas) {
    }

    public static ApiKeys forId(int id) {


Is 'short' type more suitable?

If the above comment is valid, it can be addressed in separate PR :)

We used int here because short in Java is a second class citizen and casting is required in some cases. If we decide to change this, a separate PR (as you suggested) would be better.

chia7712 · 2020-12-16T05:40:08Z

+    UPDATE_FEATURES(ApiMessageType.UPDATE_FEATURES, false, true),
+    ENVELOPE(ApiMessageType.ENVELOPE, true, RecordBatch.MAGIC_VALUE_V0, false, false);
+
+    private static final Map<Integer, ApiKeys> ID_TO_TYPE = new HashMap<>();


It seems to me this initialization can be rewrite by following code.

private static final Map<Integer, ApiKeys> ID_TO_TYPE = Arrays.stream(ApiKeys.values()) .collect(Collectors.toMap(key -> (int) key.id, Function.identity()));

The benefit is that he static block can be removed.

chia7712

+1 to latest commit.

ijuma · 2020-12-16T14:32:09Z

JDK8 and 11 passed, 15 had one flaky unrelated failure:

kafka.server.ClientQuotasRequestTest.testAlterIpQuotasRequest

chia7712 mentioned this pull request Dec 15, 2020

KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest #9741

Closed

3 tasks

chia7712 reviewed Dec 15, 2020

View reviewed changes

MINOR: Simplify ApiKeys by relying on ApiMessageType and removing unu…

ddad298

…sed methods

ijuma force-pushed the simplify-api-keys branch from 18eec5b to ddad298 Compare December 15, 2020 06:44

ijuma added 3 commits December 15, 2020 07:03

ListOffset -> ListOffsets for consistency with the existing API name

89ee566

Remove unused method

69ae211

Fix OffsetFetchResponseTest failure

53b72a8

ijuma added 2 commits December 15, 2020 07:24

Don't assume ids are dense in ApiKeys and simplify the code

0378e08

Generate highestSupportedVersion and lowestSupportedVersion

dd7e8ea

ijuma added 2 commits December 15, 2020 07:58

Replace checks in ApiKeys with unit tests in ApiMessageTypeTest

dc67bea

ijuma changed the title ~~MINOR: Simplify ApiKeys by relying on ApiMessageType and removing unused methods~~ MINOR: Simplify ApiKeys by relying on ApiMessageType Dec 15, 2020

ijuma marked this pull request as ready for review December 15, 2020 16:15

Fix compilation errors due to merge

e8dc547

ijuma force-pushed the simplify-api-keys branch from 2afb85d to e8dc547 Compare December 15, 2020 16:19

chia7712 approved these changes Dec 16, 2020

View reviewed changes

chia7712 reviewed Dec 16, 2020

View reviewed changes

Clarifying comment in ApiKeys

73099b4

chia7712 reviewed Dec 16, 2020

View reviewed changes

Simplify ID_TO_TYPE generation via stream API

d6e0f72

chia7712 approved these changes Dec 16, 2020

View reviewed changes

ijuma merged commit 782175d into apache:trunk Dec 16, 2020

ijuma deleted the simplify-api-keys branch December 16, 2020 14:33

		@@ -361,69 +172,35 @@ private static boolean shouldRetainsBufferReference(Schema[] requestSchemas) {
		}

		public static ApiKeys forId(int id) {

Conversation

ijuma commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Dec 15, 2020

Uh oh!

chia7712 commented Dec 15, 2020

Uh oh!

ijuma commented Dec 15, 2020

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Dec 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ijuma commented Dec 14, 2020 •

edited

Loading