You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2023/01/13 01:32:36 UTC
[GitHub] [kafka] jsancio opened a new pull request, #13108: KAFKA-14618; Fix off by one error in snapshot id
jsancio opened a new pull request, #13108:
URL: https://github.com/apache/kafka/pull/13108
The KRaft client expects the offset of the snapshot id to be an end offset. End offsets are exclusive. The MetadataProvenance type was createing a snapshot id using the last contained offset which is inclusive. This change fixes that and renames some of the fields to make this difference more obvious.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe merged pull request #13108: KAFKA-14618; Fix off by one error in snapshot id
Posted by GitBox <gi...@apache.org>.
cmccabe merged PR #13108:
URL: https://github.com/apache/kafka/pull/13108
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #13108: KAFKA-14618; Fix off by one error in snapshot id
Posted by GitBox <gi...@apache.org>.
cmccabe commented on PR #13108:
URL: https://github.com/apache/kafka/pull/13108#issuecomment-1382409158
I filed a follow-up jira here https://issues.apache.org/jira/browse/KAFKA-14622
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on a diff in pull request #13108: KAFKA-14618; Fix off by one error in snapshot id
Posted by GitBox <gi...@apache.org>.
jsancio commented on code in PR #13108:
URL: https://github.com/apache/kafka/pull/13108#discussion_r1068830987
##########
metadata/src/main/java/org/apache/kafka/image/MetadataProvenance.java:
##########
@@ -28,30 +29,30 @@
public final class MetadataProvenance {
public static final MetadataProvenance EMPTY = new MetadataProvenance(-1L, -1, -1L);
- private final long offset;
- private final int epoch;
+ private final long lastContainedOffset;
+ private final int lastContainedEpoch;
private final long lastContainedLogTimeMs;
public MetadataProvenance(
- long offset,
- int epoch,
+ long lastContainedOffset,
+ int lastContainedEpoch,
long lastContainedLogTimeMs
) {
- this.offset = offset;
- this.epoch = epoch;
+ this.lastContainedOffset = lastContainedOffset;
+ this.lastContainedEpoch = lastContainedEpoch;
this.lastContainedLogTimeMs = lastContainedLogTimeMs;
}
- public OffsetAndEpoch offsetAndEpoch() {
- return new OffsetAndEpoch(offset, epoch);
+ public OffsetAndEpoch snapshotId() {
+ return new OffsetAndEpoch(lastContainedOffset + 1, lastContainedEpoch);
Review Comment:
This is the most important change.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on pull request #13108: KAFKA-14618; Fix off by one error in snapshot id
Posted by GitBox <gi...@apache.org>.
jsancio commented on PR #13108:
URL: https://github.com/apache/kafka/pull/13108#issuecomment-1382578491
> Can you file a JIRA for creating a test that would have caught this?
Yes. When I implement https://issues.apache.org/jira/browse/KAFKA-14619, we can reliably catch this kind of issues in JUnit integration tests.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on a diff in pull request #13108: KAFKA-14618; Fix off by one error in snapshot id
Posted by GitBox <gi...@apache.org>.
jsancio commented on code in PR #13108:
URL: https://github.com/apache/kafka/pull/13108#discussion_r1070156218
##########
metadata/src/main/java/org/apache/kafka/image/MetadataProvenance.java:
##########
@@ -28,30 +29,30 @@
public final class MetadataProvenance {
public static final MetadataProvenance EMPTY = new MetadataProvenance(-1L, -1, -1L);
- private final long offset;
- private final int epoch;
+ private final long lastContainedOffset;
+ private final int lastContainedEpoch;
private final long lastContainedLogTimeMs;
public MetadataProvenance(
- long offset,
- int epoch,
+ long lastContainedOffset,
+ int lastContainedEpoch,
long lastContainedLogTimeMs
) {
- this.offset = offset;
- this.epoch = epoch;
+ this.lastContainedOffset = lastContainedOffset;
+ this.lastContainedEpoch = lastContainedEpoch;
this.lastContainedLogTimeMs = lastContainedLogTimeMs;
}
- public OffsetAndEpoch offsetAndEpoch() {
- return new OffsetAndEpoch(offset, epoch);
+ public OffsetAndEpoch snapshotId() {
+ return new OffsetAndEpoch(lastContainedOffset + 1, lastContainedEpoch);
Review Comment:
Yes. I'll do this when I implement https://issues.apache.org/jira/browse/KAFKA-14620.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #13108: KAFKA-14618; Fix off by one error in snapshot id
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #13108:
URL: https://github.com/apache/kafka/pull/13108#discussion_r1068983515
##########
metadata/src/main/java/org/apache/kafka/image/MetadataProvenance.java:
##########
@@ -28,30 +29,30 @@
public final class MetadataProvenance {
public static final MetadataProvenance EMPTY = new MetadataProvenance(-1L, -1, -1L);
- private final long offset;
- private final int epoch;
+ private final long lastContainedOffset;
+ private final int lastContainedEpoch;
private final long lastContainedLogTimeMs;
public MetadataProvenance(
- long offset,
- int epoch,
+ long lastContainedOffset,
+ int lastContainedEpoch,
long lastContainedLogTimeMs
) {
- this.offset = offset;
- this.epoch = epoch;
+ this.lastContainedOffset = lastContainedOffset;
+ this.lastContainedEpoch = lastContainedEpoch;
this.lastContainedLogTimeMs = lastContainedLogTimeMs;
}
- public OffsetAndEpoch offsetAndEpoch() {
- return new OffsetAndEpoch(offset, epoch);
+ public OffsetAndEpoch snapshotId() {
+ return new OffsetAndEpoch(lastContainedOffset + 1, lastContainedEpoch);
Review Comment:
Can you add a comment about how the snapshot ID contains an offset one greater than the last contained offset? It could otherwise be easy to miss.
Probably include a reference to this JIRA as well.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org