Skip to content

KAFKA-15514: Metadata records Replicas->Assignment#14516

Closed
soarez wants to merge 2 commits intoapache:trunkfrom
soarez:KAFKA-15514
Closed

KAFKA-15514: Metadata records Replicas->Assignment#14516
soarez wants to merge 2 commits intoapache:trunkfrom
soarez:KAFKA-15514

Conversation

@soarez
Copy link
Copy Markdown
Member

@soarez soarez commented Oct 10, 2023

The new "Assignments" field replaces the "Replicas" field in PartitionRecord and PartitionChangeRecord.

Depends on #14290 - KAFKA-15355

Committer Checklist (excluded from commit message)

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

The new "Assignments" field replaces the "Replicas"
field in PartitionRecord and PartitionChangeRecord.
brokers: Array[Int],
listenerName: ListenerName,
filterUnavailableEndpoints: Boolean): java.util.List[Integer] = {
// TODO KAFKA-15362
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we know what the perf impact on metadata requests is by adding the logic of KAFKA-15362 here? Especially with the warning comment in L60 regard adding extra logic to KRaftMetadataCache.maybeFilterAliveReplicas.

val assignments = partitions.asScala.map { case (partitionId, partition) =>
new TopicPartition(topicName, partitionId) ->
ReplicaAssignment(partition.replicas, partition.addingReplicas, partition.removingReplicas)
ReplicaAssignment(partition.replicaBrokerIds(), partition.addingReplicas, partition.removingReplicas)
Copy link
Copy Markdown
Contributor

@OmniaGM OmniaGM Oct 23, 2023

Choose a reason for hiding this comment

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

scala nit: It is usually recommended to not use empty parentheses in scala when a parameterless function has no side effect in both the definition of the function and when they call it (which is the case for partition.replicaBrokerIds).

val underlying = clusterInstance.asInstanceOf[ZkClusterInstance].getUnderlying()
val zkClient = underlying.zkClient
val migrationClient = ZkMigrationClient(zkClient, PasswordEncoder.noop())
val migrationClient = ZkMigrationClient(zkClient, PasswordEncoder.noop(), MetadataVersion.latest())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Same as replicaBrokerIds, MetadataVersion.latest has no side-effect so it should be called without empty parentheses.

@OmniaGM
Copy link
Copy Markdown
Contributor

OmniaGM commented Oct 24, 2023

@soarez soarez mentioned this pull request Oct 25, 2023
3 tasks
@soarez
Copy link
Copy Markdown
Member Author

soarez commented Oct 25, 2023

Thanks for the review and comments @OmniaGM . Depending on the outcome of the current discussion in #14290 there may be some bigger changes to this PR, so I'll wait until that's resolved before addressing your comments.

@soarez
Copy link
Copy Markdown
Member Author

soarez commented Nov 2, 2023

Closing this due to the change of plan in #14290

@soarez soarez closed this Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants