You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "adixitconfluent (via GitHub)" <gi...@apache.org> on 2024/11/15 13:51:51 UTC
[PR] KAFKA-18022: fetchOffsetMetadata handling for minBytes estimation in both common/uncommon cases of share fetch [kafka]
adixitconfluent opened a new pull request, #17825:
URL: https://github.com/apache/kafka/pull/17825
### About
In reference to comments https://github.com/apache/kafka/pull/17539#discussion_r1832921605 and https://github.com/apache/kafka/pull/17739#discussion_r1842631506, I have updated the way we handle fetchOffsetMetadata update and get so that minBytes estimation is consistent for both common and uncommon cases of share fetch.
### Testing
Testing has been done with the help of new/already present unit tests and already present 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
Re: [PR] KAFKA-18022: fetchOffsetMetadata handling for minBytes estimation in both common/uncommon cases of share fetch [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #17825:
URL: https://github.com/apache/kafka/pull/17825#discussion_r1844652177
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2426,4 +2421,28 @@ public String toString() {
")";
}
}
+
+ /**
+ * FetchOffsetMetadata class is used to cache offset and its log metadata.
+ */
+ static final class OffsetMetadata {
+ private long offset;
+ private LogOffsetMetadata offsetMetadata;
+
+ OffsetMetadata() {
+ }
Review Comment:
Yes, since 0 is a valid offset, it's better to initialize offset to -1.
--
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
Re: [PR] KAFKA-18022: fetchOffsetMetadata handling for minBytes estimation in both common/uncommon cases of share fetch [kafka]
Posted by "adixitconfluent (via GitHub)" <gi...@apache.org>.
adixitconfluent commented on code in PR #17825:
URL: https://github.com/apache/kafka/pull/17825#discussion_r1844556817
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2426,4 +2421,28 @@ public String toString() {
")";
}
}
+
+ /**
+ * FetchOffsetMetadata class is used to cache offset and its log metadata.
+ */
+ static final class OffsetMetadata {
+ private long offset;
+ private LogOffsetMetadata offsetMetadata;
+
+ OffsetMetadata() {
+ }
Review Comment:
I am not sure if initializing the fields has any benefits. The only value initialization inside the constructor that makes sense is `offset=-1` i.e. an invalid case. We don't really need this initialization since we can use `offsetMetadata() == null` to check if the object `fetchOffsetMetadata` has been cached yet or not.
--
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
Re: [PR] KAFKA-18022: fetchOffsetMetadata handling for minBytes estimation in both common/uncommon cases of share fetch [kafka]
Posted by "adixitconfluent (via GitHub)" <gi...@apache.org>.
adixitconfluent commented on PR #17825:
URL: https://github.com/apache/kafka/pull/17825#issuecomment-2480465213
Hi @junrao , I have pushed the suggested change. Please review this PR when you get a chance. Thanks!
--
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
Re: [PR] KAFKA-18022: fetchOffsetMetadata handling for minBytes estimation in both common/uncommon cases of share fetch [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #17825:
URL: https://github.com/apache/kafka/pull/17825#discussion_r1844359245
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2426,4 +2421,28 @@ public String toString() {
")";
}
}
+
+ /**
+ * FetchOffsetMetadata class is used to cache offset and its log metadata.
+ */
+ static final class OffsetMetadata {
+ private long offset;
+ private LogOffsetMetadata offsetMetadata;
+
+ OffsetMetadata() {
+ }
Review Comment:
Should we initialize the fields?
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2426,4 +2421,28 @@ public String toString() {
")";
}
}
+
+ /**
+ * FetchOffsetMetadata class is used to cache offset and its log metadata.
+ */
+ static final class OffsetMetadata {
+ private long offset;
Review Comment:
It's useful to add a comment that this offset could be different from offsetMetadata.messageOffset if it's in the middle of a batch.
--
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
Re: [PR] KAFKA-18022: fetchOffsetMetadata handling for minBytes estimation in both common/uncommon cases of share fetch [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao merged PR #17825:
URL: https://github.com/apache/kafka/pull/17825
--
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
Re: [PR] KAFKA-18022: fetchOffsetMetadata handling for minBytes estimation in both common/uncommon cases of share fetch [kafka]
Posted by "adixitconfluent (via GitHub)" <gi...@apache.org>.
adixitconfluent commented on code in PR #17825:
URL: https://github.com/apache/kafka/pull/17825#discussion_r1844656641
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2426,4 +2421,28 @@ public String toString() {
")";
}
}
+
+ /**
+ * FetchOffsetMetadata class is used to cache offset and its log metadata.
+ */
+ static final class OffsetMetadata {
+ private long offset;
+ private LogOffsetMetadata offsetMetadata;
+
+ OffsetMetadata() {
+ }
Review Comment:
i've pushed the changes in my latest commit. Thanks!
--
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
Re: [PR] KAFKA-18022: fetchOffsetMetadata handling for minBytes estimation in both common/uncommon cases of share fetch [kafka]
Posted by "adixitconfluent (via GitHub)" <gi...@apache.org>.
adixitconfluent commented on code in PR #17825:
URL: https://github.com/apache/kafka/pull/17825#discussion_r1844656641
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2426,4 +2421,28 @@ public String toString() {
")";
}
}
+
+ /**
+ * FetchOffsetMetadata class is used to cache offset and its log metadata.
+ */
+ static final class OffsetMetadata {
+ private long offset;
+ private LogOffsetMetadata offsetMetadata;
+
+ OffsetMetadata() {
+ }
Review Comment:
i've pushed the change in my latest commit. Thanks!
--
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