Skip to content

KAFKA-12212; Bump Metadata API version to remove ClusterAuthorizedOperations fields#9945

Merged
dajac merged 2 commits intoapache:trunkfrom
dajac:KAFKA-12212
Jan 22, 2021
Merged

KAFKA-12212; Bump Metadata API version to remove ClusterAuthorizedOperations fields#9945
dajac merged 2 commits intoapache:trunkfrom
dajac:KAFKA-12212

Conversation

@dajac
Copy link
Copy Markdown
Member

@dajac dajac commented Jan 21, 2021

This PR bumps the version of the Metadata API and deprecates the IncludeClusterAuthorizedOperations and the IncludeClusterAuthorizedOperations fields from version 11 and onward.

Committer Checklist (excluded from commit message)

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

@dajac dajac marked this pull request as ready for review January 21, 2021 10:48
@dajac dajac requested a review from rajinisivaram January 21, 2021 10:48
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dajac thanks for your patch!

@@ -1246,13 +1246,15 @@ class KafkaApis(val requestChannel: RequestChannel,
}

var clusterAuthorizedOperations = Int.MinValue
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the default value of clusterAuthorizedOperations in auto-generated protocol is -2147483648. If the version of request is bigger than 10, does it cause error if we set Int.MinValue to clusterAuthorizedOperations of MetadataResponseData?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-2147483648 is actually Int.MinValue. It would cause an error if we would write something different from the default value for versions < 8 and > 10.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the comments for that value (Int.MinValue) for dumb readers like me :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

//
// Version 10 adds topicId.
//
// Version 11 deprecates IncludeClusterAuthorizedOperations field (KIP-700).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about saying this function is migrated to DescribeClusterRequest?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, why not. I will add it.

@dajac
Copy link
Copy Markdown
Member Author

dajac commented Jan 21, 2021

@chia7712 Thanks for your comments. I have updated the PR to address them.

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dajac Thanks for your patch. +1

Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dajac Thanks for the PR, LGTM

@dajac
Copy link
Copy Markdown
Member Author

dajac commented Jan 22, 2021

Failed tests are not related:

  • Build / JDK 8 / org.apache.kafka.clients.consumer.KafkaConsumerTest.testCloseWithTimeUnit()
  • Build / JDK 8 / org.apache.kafka.clients.consumer.internals.FetcherTest.testEarlierOffsetResetArrivesLate()

Merging to trunk.

@dajac dajac merged commit 7a1d1d9 into apache:trunk Jan 22, 2021
@dajac dajac deleted the KAFKA-12212 branch January 22, 2021 08:06
ijuma added a commit to ijuma/kafka that referenced this pull request Jan 26, 2021
…e-allocations-lz4

* apache-github/trunk: (562 commits)
  MINOR: remove unused code from MessageTest (apache#9961)
  MINOR: Fix visibility of Log.{unflushedMessages, addSegment} methods (apache#9966)
  KAFKA-12229: Restore original class loader in integration tests using EmbeddedConnectCluster during shutdown  (apache#9942)
  KAFKA-12190: Fix setting of file permissions on non-POSIX filesystems (apache#9947)
  MINOR: Remove `toStruct` and `fromStruct` methods from generated protocol classes (apache#9960)
  MINOR: Fix typo in Utils#toPositive (apache#9943)
  MINOR: MessageUtil: remove some deadcode (apache#9931)
  MINOR: Update zstd-jni to 1.4.8-2 (apache#9957)
  MINOR: Revert assertion in MockProducerTest (apache#9956)
  MINOR: Optimize assertions in unit tests (apache#9955)
  MINOR: Tag `RaftEventSimulationTest` as `integration` and tweak it (apache#9925)
  MINOR: Update to Gradle 6.8.1 (apache#9953)
  MINOR: A few small group coordinator cleanups (apache#9952)
  MINOR: Upgrade ducktape to version 0.8.1  (apache#9933)
  MINOR: fix record time in test shouldWipeOutStandbyStateDirectoryIfCheckpointIsMissing (apache#9948)
  MINOR: Restore interrupt status when closing (apache#9863)
  KAFKA-10357: Extract setup of repartition topics from Streams partition assignor (apache#9848)
  KAFKA-12212; Bump Metadata API version to remove `ClusterAuthorizedOperations` fields (KIP-700) (apache#9945)
  MINOR: log 2min processing summary of StreamThread loop (apache#9941)
  MINOR: Drop enable.metadata.quorum config (apache#9934)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants