artemlivshits commented on code in PR #19163:
URL: https://github.com/apache/kafka/pull/19163#discussion_r1987985812
##########
clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java:
##########
@@ -122,8 +124,8 @@ public final ByteBuffer serializeWithHeader(RequestHeader
header) {
}
// Visible for testing
- public final ByteBuffer serialize() {
- return MessageUtil.toByteBuffer(data(), version);
+ public final Readable serialize() {
Review Comment:
Then probably it would make sense to make `toByteBufferAccessor` public, and
I'm not sure if we need `toReadable` and `toByteBuffer` overloads as the data
can be easily extracted from `ByteBufferAccessor`.
##########
clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java:
##########
@@ -168,190 +175,194 @@ public Map<Errors, Integer> errorCounts(Throwable e) {
* Factory method for getting a request object based on ApiKey ID and a
version
*/
public static RequestAndSize parseRequest(ApiKeys apiKey, short
apiVersion, ByteBuffer buffer) {
- int bufferSize = buffer.remaining();
- return new RequestAndSize(doParseRequest(apiKey, apiVersion, buffer),
bufferSize);
+ return parseRequest(apiKey, apiVersion, new
ByteBufferAccessor(buffer));
Review Comment:
Do we need an overload that takes a ByteBuffer? I found one case (see my
other comment), but it could be easily fixed if `serialize()` returned
`ByteBufferAccessor`.
##########
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java:
##########
@@ -1923,9 +1922,9 @@ private void checkRequest(AbstractRequest req) {
// Check for equality of the ByteBuffer only if indicated (it is
likely to fail if any of the fields
// in the request is a HashMap with multiple elements since ordering
of the elements may vary)
try {
- ByteBuffer serializedBytes = req.serialize();
+ ByteBuffer serializedBytes = req.serializeToByteBuffer();
Review Comment:
If `req.serialize()` returned `ByteBufferAccessor`, we could use that here.
The benefit is that we could just use `parseRequest` that takes `Readable`.
##########
clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java:
##########
@@ -122,8 +124,8 @@ public final ByteBuffer serializeWithHeader(RequestHeader
header) {
}
// Visible for testing
- public final ByteBuffer serialize() {
- return MessageUtil.toByteBuffer(data(), version);
+ public final Readable serialize() {
Review Comment:
I was kind of thinking maybe it should just return `ByteAccessor` without
having another overload? All the code that takes `Readable` would just work,
and a small number of places that need to work with the buffer could use the
result as the `ByteAccessor`. Pretty much similar to what you had with
downcasting only without downcasting :-).
--
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
