Re: [PR] KAFKA-16855 : Part 1 - New fields tieredEpoch and tieredState [kafka]
kamalcph commented on code in PR #16257:
URL: https://github.com/apache/kafka/pull/16257#discussion_r1687395435
##
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogSegmentMetadata.java:
##
@@ -104,10 +110,11 @@ public RemoteLogSegmentMetadata(RemoteLogSegmentId
remoteLogSegmentId,
int segmentSizeInBytes,
Optional customMetadata,
RemoteLogSegmentState state,
-Map segmentLeaderEpochs) {
+Map segmentLeaderEpochs,
int tieredEpoch) {
Review Comment:
you can address them in your next part-2 PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16855 : Part 1 - New fields tieredEpoch and tieredState [kafka]
muralibasani commented on code in PR #16257:
URL: https://github.com/apache/kafka/pull/16257#discussion_r1687093271
##
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogSegmentMetadata.java:
##
@@ -104,10 +110,11 @@ public RemoteLogSegmentMetadata(RemoteLogSegmentId
remoteLogSegmentId,
int segmentSizeInBytes,
Optional customMetadata,
RemoteLogSegmentState state,
-Map segmentLeaderEpochs) {
+Map segmentLeaderEpochs,
int tieredEpoch) {
Review Comment:
@kamalcph as this pr was merged last month, do you suggest to create a new
jira/pr for your suggested changes ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16855 : Part 1 - New fields tieredEpoch and tieredState [kafka]
kamalcph commented on code in PR #16257:
URL: https://github.com/apache/kafka/pull/16257#discussion_r1686572470
##
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogSegmentMetadata.java:
##
@@ -241,7 +255,7 @@ public RemoteLogSegmentMetadata
createWithUpdates(RemoteLogSegmentMetadataUpdate
return new RemoteLogSegmentMetadata(remoteLogSegmentId, startOffset,
endOffset, maxTimestampMs, rlsmUpdate.brokerId(),
rlsmUpdate.eventTimestampMs(),
-segmentSizeInBytes, rlsmUpdate.customMetadata(),
rlsmUpdate.state(), segmentLeaderEpochs);
+segmentSizeInBytes, rlsmUpdate.customMetadata(),
rlsmUpdate.state(), segmentLeaderEpochs, tieredEpoch);
Review Comment:
should we also update `hashcode` and `equals` with tieredEpoch field?
##
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogSegmentMetadata.java:
##
@@ -104,10 +110,11 @@ public RemoteLogSegmentMetadata(RemoteLogSegmentId
remoteLogSegmentId,
int segmentSizeInBytes,
Optional customMetadata,
RemoteLogSegmentState state,
-Map segmentLeaderEpochs) {
+Map segmentLeaderEpochs,
int tieredEpoch) {
Review Comment:
nit: newline
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogSegmentMetadataTransform.java:
##
@@ -83,7 +83,7 @@ public RemoteLogSegmentMetadata
fromApiMessageAndVersion(ApiMessageAndVersion ap
new RemoteLogSegmentMetadata(remoteLogSegmentId,
record.startOffset(), record.endOffset(),
record.maxTimestampMs(),
record.brokerId(),
record.eventTimestampMs(),
record.segmentSizeInBytes(),
- segmentLeaderEpochs);
+ segmentLeaderEpochs, 0);
Review Comment:
Why we didn't made similar change in `toApiMessageAndVersion` method?
##
metadata/src/main/resources/common/metadata/TopicRecord.json:
##
@@ -17,12 +17,16 @@
"apiKey": 2,
"type": "metadata",
"name": "TopicRecord",
- "validVersions": "0",
+ "validVersions": "0-1",
Review Comment:
For my understanding:
If the `__cluster_metadata` topic contains the `TopicRecord` message with
both the versions 0 and 1. Will the brokers with old binary able to deserialize
them?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16855 : Part 1 - New fields tieredEpoch and tieredState [kafka]
muralibasani commented on PR #16257: URL: https://github.com/apache/kafka/pull/16257#issuecomment-2208871103 > > @clolov : This needs a new metadata version. Can you please create one, or roll this back? > > Thanks for the reminder, @cmccabe ! [KAFKA-17080](https://issues.apache.org/jira/browse/KAFKA-17080) is created for this. @clolov @muralibasani , FYI. Thanks, I will look into it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16855 : Part 1 - New fields tieredEpoch and tieredState [kafka]
showuon commented on PR #16257: URL: https://github.com/apache/kafka/pull/16257#issuecomment-2208749158 > @clolov : This needs a new metadata version. Can you please create one, or roll this back? Thanks for the reminder, @cmccabe ! [KAFKA-17080](https://issues.apache.org/jira/browse/KAFKA-17080) is created for this. @clolov @muralibasani , FYI. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16855 : Part 1 - New fields tieredEpoch and tieredState [kafka]
muralibasani commented on PR #16257: URL: https://github.com/apache/kafka/pull/16257#issuecomment-2186577649 > Heya @muralibasani, could you just fix > > ``` > [2024-06-21T10:19:19.373Z] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-16257/core/src/test/java/kafka/log/remote/RemoteLogManagerTest.java:2105: error: no suitable constructor found for RemoteLogSegmentMetadata(RemoteLogSegmentId,long,long,long,int,long,int,Optional,RemoteLogSegmentState,NavigableMap) > ``` > > from the build? Seems like a new test was added. Updated it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16855 : Part 1 - New fields tieredEpoch and tieredState [kafka]
clolov commented on PR #16257: URL: https://github.com/apache/kafka/pull/16257#issuecomment-2186519073 Heya @muralibasani, could you just fix ``` [2024-06-21T10:19:19.373Z] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-16257/core/src/test/java/kafka/log/remote/RemoteLogManagerTest.java:2105: error: no suitable constructor found for RemoteLogSegmentMetadata(RemoteLogSegmentId,long,long,long,int,long,int,Optional,RemoteLogSegmentState,NavigableMap) ``` from the build? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16855 : Part 1 - New fields tieredEpoch and tieredState [kafka]
clolov commented on code in PR #16257: URL: https://github.com/apache/kafka/pull/16257#discussion_r1650984849 ## metadata/src/main/resources/common/metadata/TopicRecord.json: ## @@ -17,12 +17,16 @@ "apiKey": 2, "type": "metadata", "name": "TopicRecord", - "validVersions": "0", + "validVersions": "0-1", Review Comment: I see, I have not fully caught up on KIP-1022, which appears to have changed the approach for unstable metadata version. Okay, let's continue with what is proposed in this PR. Thank you for checking! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] KAFKA-16855 : Part 1 - New fields tieredEpoch and tieredState [kafka]
muralibasani opened a new pull request, #16257: URL: https://github.com/apache/kafka/pull/16257 Resolves : https://issues.apache.org/jira/browse/KAFKA-16855 - Add field tieredEpoch to RemoteLogSegmentMetadata - Update relevant tests - Add two fields tieredEpoch and tieredState to TopicRecord.json ### Committer Checklist (excluded from commit message) - [X] Verify design and implementation - [X] Verify test coverage and CI build status - [X] Verify documentation (including upgrade notes) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
