You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "luozhenyu (via GitHub)" <gi...@apache.org> on 2024/05/13 15:16:42 UTC
[PR] KAFKA-16738: Returns BaseRecords instead of MemoryRecords [kafka]
luozhenyu opened a new pull request, #15935:
URL: https://github.com/apache/kafka/pull/15935
Related to https://issues.apache.org/jira/browse/KAFKA-16738
We can write a record which is a subtype of [BaseRecords](https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/record/BaseRecords.java), but we can not read a record which is a subtype of [BaseRecords](https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/record/BaseRecords.java). If we change the return type of [Readable#readRecords](https://github.com/apache/kafka/blob/5439914c32fa00d634efa7219699f1bc21add839/clients/src/main/java/org/apache/kafka/common/protocol/Readable.java#L56) from MemoryRecords to BaseRecords, we can override the implementation of readRecords easily.
We known that the MemoryRecords is based on JDK's ByteBuffer. We are developing a netty project([kroxylicious](https://github.com/kroxylicious/kroxylicious/)) and we want to create a subtype of BaseRecords like MemoryRecords based on netty's ByteBuf.
### 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16738: Returns BaseRecords instead of MemoryRecords [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15935:
URL: https://github.com/apache/kafka/pull/15935#discussion_r1598750305
##########
clients/src/main/java/org/apache/kafka/common/protocol/Readable.java:
##########
@@ -53,7 +54,7 @@ default List<RawTaggedField> readUnknownTaggedField(List<RawTaggedField> unknown
return unknowns;
}
- default MemoryRecords readRecords(int length) {
+ default BaseRecords readRecords(int length) {
Review Comment:
in kafak serialization, we assume the impl of `BaseRecords` is `MemoryRecords`. For example:
```java
@Override
public PartitionProduceData duplicate() {
PartitionProduceData _duplicate = new PartitionProduceData();
_duplicate.index = index;
if (records == null) {
_duplicate.records = null;
} else {
_duplicate.records = MemoryRecords.readableRecords(((MemoryRecords) records).buffer().duplicate());
}
return _duplicate;
}
```
Hence, I'm not sure how this change works if you introduce a non-MemoryRecords impl.
--
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-16738: Returns BaseRecords instead of MemoryRecords [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15935:
URL: https://github.com/apache/kafka/pull/15935#issuecomment-2571488751
This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.
--
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-16738: Returns BaseRecords instead of MemoryRecords [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15935:
URL: https://github.com/apache/kafka/pull/15935#issuecomment-2297941053
This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch) <p> If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.
--
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-16738: Returns BaseRecords instead of MemoryRecords [kafka]
Posted by "luozhenyu (via GitHub)" <gi...@apache.org>.
luozhenyu commented on code in PR #15935:
URL: https://github.com/apache/kafka/pull/15935#discussion_r1599493821
##########
clients/src/main/java/org/apache/kafka/common/protocol/Readable.java:
##########
@@ -53,7 +54,7 @@ default List<RawTaggedField> readUnknownTaggedField(List<RawTaggedField> unknown
return unknowns;
}
- default MemoryRecords readRecords(int length) {
+ default BaseRecords readRecords(int length) {
Review Comment:
> Hence, I'm not sure how this change works if you introduce a non-MemoryRecords impl.
Hard-coded duplicating of `MemoryRecords` is not elegant. So I add a `duplicate` method to BaseRecords. How do you think?
```java
@Override
public PartitionProduceData duplicate() {
PartitionProduceData _duplicate = new PartitionProduceData();
_duplicate.index = index;
if (records == null) {
_duplicate.records = null;
} else {
_duplicate.records = records.duplicate();
}
return _duplicate;
}
```
--
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-16738: Returns BaseRecords instead of MemoryRecords [kafka]
Posted by "luozhenyu (via GitHub)" <gi...@apache.org>.
luozhenyu commented on code in PR #15935:
URL: https://github.com/apache/kafka/pull/15935#discussion_r1599493821
##########
clients/src/main/java/org/apache/kafka/common/protocol/Readable.java:
##########
@@ -53,7 +54,7 @@ default List<RawTaggedField> readUnknownTaggedField(List<RawTaggedField> unknown
return unknowns;
}
- default MemoryRecords readRecords(int length) {
+ default BaseRecords readRecords(int length) {
Review Comment:
> Hence, I'm not sure how this change works if you introduce a non-MemoryRecords impl.
Hard-coded duplicating of `MemoryRecords` is not elegant. So I add a `duplicate` method to BaseRecords. How do you think?
--
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-16738: Returns BaseRecords instead of MemoryRecords [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15935:
URL: https://github.com/apache/kafka/pull/15935#discussion_r1600107425
##########
clients/src/main/java/org/apache/kafka/common/protocol/Readable.java:
##########
@@ -53,7 +54,7 @@ default List<RawTaggedField> readUnknownTaggedField(List<RawTaggedField> unknown
return unknowns;
}
- default MemoryRecords readRecords(int length) {
+ default BaseRecords readRecords(int length) {
Review Comment:
> Hard-coded duplicating of MemoryRecords is not elegant. So I add a duplicate method to BaseRecords. How do you think?
Could you file a KIP to let us know the whole picture? `MemoryRecords` is used in code base, so we need to be careful of changing that.
--
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-16738: Returns BaseRecords instead of MemoryRecords [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #15935: KAFKA-16738: Returns BaseRecords instead of MemoryRecords
URL: https://github.com/apache/kafka/pull/15935
--
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