-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-18303; Update ShareCoordinator to use new record format #18396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
|
|
||
| import org.apache.kafka.common.Uuid; | ||
| import org.apache.kafka.coordinator.common.runtime.CoordinatorRecord; | ||
| import org.apache.kafka.coordinator.share.generated.CoordinatorRecordType; | ||
| import org.apache.kafka.coordinator.share.generated.ShareSnapshotKey; | ||
| import org.apache.kafka.coordinator.share.generated.ShareSnapshotValue; | ||
| import org.apache.kafka.coordinator.share.generated.ShareUpdateKey; | ||
|
|
@@ -33,7 +34,7 @@ public static CoordinatorRecord newShareSnapshotRecord(String groupId, Uuid topi | |
| .setGroupId(groupId) | ||
| .setTopicId(topicId) | ||
| .setPartition(partitionId), | ||
| ShareCoordinator.SHARE_SNAPSHOT_RECORD_KEY_VERSION), | ||
| CoordinatorRecordType.SHARE_SNAPSHOT.id()), | ||
| new ApiMessageAndVersion(new ShareSnapshotValue() | ||
| .setSnapshotEpoch(offsetData.snapshotEpoch()) | ||
| .setStateEpoch(offsetData.stateEpoch()) | ||
|
|
@@ -46,7 +47,7 @@ public static CoordinatorRecord newShareSnapshotRecord(String groupId, Uuid topi | |
| .setDeliveryCount(batch.deliveryCount()) | ||
| .setDeliveryState(batch.deliveryState())) | ||
| .collect(Collectors.toList())), | ||
| ShareCoordinator.SHARE_SNAPSHOT_RECORD_VALUE_VERSION) | ||
| (short) 0) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is better to hardcode the version here in order to avoid using the wrong ones as it was the case. |
||
| ); | ||
| } | ||
|
|
||
|
|
@@ -56,7 +57,7 @@ public static CoordinatorRecord newShareSnapshotUpdateRecord(String groupId, Uui | |
| .setGroupId(groupId) | ||
| .setTopicId(topicId) | ||
| .setPartition(partitionId), | ||
| ShareCoordinator.SHARE_UPDATE_RECORD_KEY_VERSION), | ||
| CoordinatorRecordType.SHARE_UPDATE.id()), | ||
| new ApiMessageAndVersion(new ShareUpdateValue() | ||
| .setSnapshotEpoch(offsetData.snapshotEpoch()) | ||
| .setLeaderEpoch(offsetData.leaderEpoch()) | ||
|
|
@@ -68,7 +69,7 @@ public static CoordinatorRecord newShareSnapshotUpdateRecord(String groupId, Uui | |
| .setDeliveryCount(batch.deliveryCount()) | ||
| .setDeliveryState(batch.deliveryState())) | ||
| .collect(Collectors.toList())), | ||
| ShareCoordinator.SHARE_UPDATE_RECORD_VALUE_VERSION) | ||
| (short) 0) | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,9 +29,9 @@ public class ShareCoordinatorRecordSerde extends CoordinatorRecordSerde { | |
| @Override | ||
| protected ApiMessage apiMessageKeyFor(short recordVersion) { | ||
| switch (recordVersion) { | ||
| case ShareCoordinator.SHARE_SNAPSHOT_RECORD_KEY_VERSION: | ||
| case 0: | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a temporary step backward. I will rework it in https://issues.apache.org/jira/browse/KAFKA-18308. |
||
| return new ShareSnapshotKey(); | ||
| case ShareCoordinator.SHARE_UPDATE_RECORD_KEY_VERSION: | ||
| case 1: | ||
| return new ShareUpdateKey(); | ||
| default: | ||
| throw new CoordinatorLoader.UnknownRecordTypeException(recordVersion); | ||
|
|
@@ -41,9 +41,9 @@ protected ApiMessage apiMessageKeyFor(short recordVersion) { | |
| @Override | ||
| protected ApiMessage apiMessageValueFor(short recordVersion) { | ||
| switch (recordVersion) { | ||
| case ShareCoordinator.SHARE_SNAPSHOT_RECORD_VALUE_VERSION: | ||
| case 0: | ||
| return new ShareSnapshotValue(); | ||
| case ShareCoordinator.SHARE_UPDATE_RECORD_VALUE_VERSION: | ||
| case 1: | ||
| return new ShareUpdateValue(); | ||
| default: | ||
| throw new CoordinatorLoader.UnknownRecordTypeException(recordVersion); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
|
|
||
| import org.apache.kafka.common.TopicPartition; | ||
| import org.apache.kafka.common.Uuid; | ||
| import org.apache.kafka.common.errors.UnsupportedVersionException; | ||
| import org.apache.kafka.common.message.ReadShareGroupStateRequestData; | ||
| import org.apache.kafka.common.message.ReadShareGroupStateResponseData; | ||
| import org.apache.kafka.common.message.WriteShareGroupStateRequestData; | ||
|
|
@@ -38,6 +39,7 @@ | |
| import org.apache.kafka.coordinator.common.runtime.CoordinatorShard; | ||
| import org.apache.kafka.coordinator.common.runtime.CoordinatorShardBuilder; | ||
| import org.apache.kafka.coordinator.common.runtime.CoordinatorTimer; | ||
| import org.apache.kafka.coordinator.share.generated.CoordinatorRecordType; | ||
| import org.apache.kafka.coordinator.share.generated.ShareSnapshotKey; | ||
| import org.apache.kafka.coordinator.share.generated.ShareSnapshotValue; | ||
| import org.apache.kafka.coordinator.share.generated.ShareUpdateKey; | ||
|
|
@@ -206,15 +208,19 @@ public void replay(long offset, long producerId, short producerEpoch, Coordinato | |
| ApiMessageAndVersion key = record.key(); | ||
| ApiMessageAndVersion value = record.value(); | ||
|
|
||
| switch (key.version()) { | ||
| case ShareCoordinator.SHARE_SNAPSHOT_RECORD_KEY_VERSION: // ShareSnapshot | ||
| handleShareSnapshot((ShareSnapshotKey) key.message(), (ShareSnapshotValue) messageOrNull(value), offset); | ||
| break; | ||
| case ShareCoordinator.SHARE_UPDATE_RECORD_KEY_VERSION: // ShareUpdate | ||
| handleShareUpdate((ShareUpdateKey) key.message(), (ShareUpdateValue) messageOrNull(value)); | ||
| break; | ||
| default: | ||
| // Noop | ||
| try { | ||
| switch (CoordinatorRecordType.fromId(key.version())) { | ||
| case SHARE_SNAPSHOT: | ||
| handleShareSnapshot((ShareSnapshotKey) key.message(), (ShareSnapshotValue) messageOrNull(value), offset); | ||
| break; | ||
| case SHARE_UPDATE: | ||
| handleShareUpdate((ShareUpdateKey) key.message(), (ShareUpdateValue) messageOrNull(value)); | ||
| break; | ||
| default: | ||
| // Noop | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we can use java 17 enhanced switch to eliminate unnecessary default branch.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a separate PR for doing this. |
||
| } | ||
| } catch (UnsupportedVersionException ex) { | ||
| // Ignore | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ignore unknown records to follow the current implementation. |
||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug. The version of the update value record must be 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndrewJSchofield Is this something that we want to fix in 4.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pardon me, why not fix it for 4.0? I assume it would be an issue to 4.1 in reading the records having incorrect version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't support migration of the share-group state topic data from 4.0 to 4.1, so not strictly necessary. But really, it is not ideal. Probably should fix it. I'll open an issue.