[jira] [Commented] (KAFKA-1644) Inherit FetchResponse from RequestOrResponse
[ https://issues.apache.org/jira/browse/KAFKA-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14165287#comment-14165287 ] Anton Karamanov commented on KAFKA-1644: That's great news, thanks. > Inherit FetchResponse from RequestOrResponse > > > Key: KAFKA-1644 > URL: https://issues.apache.org/jira/browse/KAFKA-1644 > Project: Kafka > Issue Type: Improvement >Reporter: Anton Karamanov >Assignee: Anton Karamanov > Fix For: 0.8.2, 0.8.3 > > Attachments: > 0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch, > 0002-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch, > 0003-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch > > > Unlike all other Kafka API responses {{FetchResponse}} is not a subclass of > RequestOrResponse, which requires handling it as a special case while > processing responses. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-1644) Inherit FetchResponse from RequestOrResponse
[ https://issues.apache.org/jira/browse/KAFKA-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14165278#comment-14165278 ] Jun Rao commented on KAFKA-1644: Ok, since this a small patch, I double committed to 0.8.2. > Inherit FetchResponse from RequestOrResponse > > > Key: KAFKA-1644 > URL: https://issues.apache.org/jira/browse/KAFKA-1644 > Project: Kafka > Issue Type: Improvement >Reporter: Anton Karamanov >Assignee: Anton Karamanov > Fix For: 0.8.2, 0.8.3 > > Attachments: > 0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch, > 0002-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch, > 0003-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch > > > Unlike all other Kafka API responses {{FetchResponse}} is not a subclass of > RequestOrResponse, which requires handling it as a special case while > processing responses. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-1644) Inherit FetchResponse from RequestOrResponse
[ https://issues.apache.org/jira/browse/KAFKA-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14163449#comment-14163449 ] Anton Karamanov commented on KAFKA-1644: Thanks. Is there a chance it could be included in 0.8.2 release? > Inherit FetchResponse from RequestOrResponse > > > Key: KAFKA-1644 > URL: https://issues.apache.org/jira/browse/KAFKA-1644 > Project: Kafka > Issue Type: Improvement >Reporter: Anton Karamanov >Assignee: Anton Karamanov > Fix For: 0.8.3 > > Attachments: > 0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch, > 0002-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch, > 0003-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch > > > Unlike all other Kafka API responses {{FetchResponse}} is not a subclass of > RequestOrResponse, which requires handling it as a special case while > processing responses. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-1644) Inherit FetchResponse from RequestOrResponse
[ https://issues.apache.org/jira/browse/KAFKA-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14161187#comment-14161187 ] Jun Rao commented on KAFKA-1644: Perhaps it's better to throw a UnsupportedOperationExceptions in writeTo()? > Inherit FetchResponse from RequestOrResponse > > > Key: KAFKA-1644 > URL: https://issues.apache.org/jira/browse/KAFKA-1644 > Project: Kafka > Issue Type: Improvement >Reporter: Anton Karamanov >Assignee: Anton Karamanov > Attachments: > 0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch, > 0002-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch > > > Unlike all other Kafka API responses {{FetchResponse}} is not a subclass of > RequestOrResponse, which requires handling it as a special case while > processing responses. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-1644) Inherit FetchResponse from RequestOrResponse
[ https://issues.apache.org/jira/browse/KAFKA-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14159758#comment-14159758 ] Jun Rao commented on KAFKA-1644: Alexey, My only concern of the patch is that it gives people the impression that FetchResponse.writeTo() is supported in the same way as other requests/responses by serializing the full request/response into a ByteBuffer. which can be misleading. > Inherit FetchResponse from RequestOrResponse > > > Key: KAFKA-1644 > URL: https://issues.apache.org/jira/browse/KAFKA-1644 > Project: Kafka > Issue Type: Improvement >Reporter: Anton Karamanov >Assignee: Anton Karamanov > Attachments: > 0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch > > > Unlike all other Kafka API responses {{FetchResponse}} is not a subclass of > RequestOrResponse, which requires handling it as a special case while > processing responses. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-1644) Inherit FetchResponse from RequestOrResponse
[ https://issues.apache.org/jira/browse/KAFKA-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14158114#comment-14158114 ] Alexey Ozeritskiy commented on KAFKA-1644: -- This patch simplifies writing clients. Now we have to use the following ugly code: {code:java} if (id == ResponseKeys.FetchKey) { val response = FetchResponse.readFrom(contentBuffer) listener ! FetchAnswer(response) } else { val response = ResponseKeys.deserializerForKey(id)(contentBuffer) listener ! Answer(response) } {code} > Inherit FetchResponse from RequestOrResponse > > > Key: KAFKA-1644 > URL: https://issues.apache.org/jira/browse/KAFKA-1644 > Project: Kafka > Issue Type: Bug >Reporter: Anton Karamanov >Assignee: Anton Karamanov > Attachments: > 0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch > > > Unlike all other Kafka API responses {{FetchResponse}} is not a subclass of > RequestOrResponse, which requires handling it as a special case while > processing responses. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-1644) Inherit FetchResponse from RequestOrResponse
[ https://issues.apache.org/jira/browse/KAFKA-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14156156#comment-14156156 ] Joe Stein commented on KAFKA-1644: -- Unless it is a bug or defect I don't see the point to change code. We could add it to some refactoring list maybe for 1.0? > Inherit FetchResponse from RequestOrResponse > > > Key: KAFKA-1644 > URL: https://issues.apache.org/jira/browse/KAFKA-1644 > Project: Kafka > Issue Type: Bug >Reporter: Anton Karamanov >Assignee: Anton Karamanov > Attachments: > 0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch > > > Unlike all other Kafka API responses {{FetchResponse}} is not a subclass of > RequestOrResponse, which requires handling it as a special case while > processing responses. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-1644) Inherit FetchResponse from RequestOrResponse
[ https://issues.apache.org/jira/browse/KAFKA-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14156149#comment-14156149 ] Anton Karamanov commented on KAFKA-1644: Jun, no {{FetchResponseSend}} still uses sendfile logic. {{FetchResponse.writeTo()}} only writes a fixed sized fetch message header to buffer. {code} - buffer.putInt(size) - buffer.putInt(fetchResponse.correlationId) - buffer.putInt(fetchResponse.dataGroupedByTopic.size) // topic count + fetchResponse.writeTo(buffer) {code} Guozhang, indeed it is not, it's priority can be lowered if necessary. But it's a pretty simple change too. The problem you are describing with {{FetchResponse.writeTo}} not being semantically consistent with that of other API messages is the very same I've mentioned as my concern. However, it seems like an OK compromise to me to use it for writing fixed-size message header, which depends only on {{FetchResponse}} members ({{correlationId}} and size of {{dataGroupedByTopic}}) instead of making it to do nothing at all just to conform to required function signature. My primary goal for this task is to make {{FetchResponse}} to be a part of common superclass to simplify client-side message flow control. Maybe another trait can be introduced without any abstract members, just to mark all Kafka API messages. Or {{RequestOrResponse}} can be split into {{RequestOrResponse}} without {{writeTo}} abstract method and something like {{Writable}} trait which will require to provide it. > Inherit FetchResponse from RequestOrResponse > > > Key: KAFKA-1644 > URL: https://issues.apache.org/jira/browse/KAFKA-1644 > Project: Kafka > Issue Type: Bug >Reporter: Anton Karamanov >Assignee: Anton Karamanov > Attachments: > 0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch > > > Unlike all other Kafka API responses {{FetchResponse}} is not a subclass of > RequestOrResponse, which requires handling it as a special case while > processing responses. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-1644) Inherit FetchResponse from RequestOrResponse
[ https://issues.apache.org/jira/browse/KAFKA-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14155903#comment-14155903 ] Jun Rao commented on KAFKA-1644: Anton, By letting FetchResponseSend to call FetchResponse.writeTo(), you are no longer using the sendfile logic, right? > Inherit FetchResponse from RequestOrResponse > > > Key: KAFKA-1644 > URL: https://issues.apache.org/jira/browse/KAFKA-1644 > Project: Kafka > Issue Type: Bug >Reporter: Anton Karamanov >Assignee: Anton Karamanov > Attachments: > 0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch > > > Unlike all other Kafka API responses {{FetchResponse}} is not a subclass of > RequestOrResponse, which requires handling it as a special case while > processing responses. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-1644) Inherit FetchResponse from RequestOrResponse
[ https://issues.apache.org/jira/browse/KAFKA-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14155678#comment-14155678 ] Guozhang Wang commented on KAFKA-1644: -- Anton, I think Jun's not saying that there is a blocker for doing so, it's just that since FetchResponse does not use writeTo to write to the socket, but use FetchReponseSend instead for zero-copy, it may not logically be treated as inheritance of RequestOrResponse. After looking into your patch I see you are enforcing this by letting FetchResponseSend to just call FetchResponse.writeTo instead, which I think is OK if we do have a strong reason for letting FetchResponse inheriting from RequestOrResponse. > Inherit FetchResponse from RequestOrResponse > > > Key: KAFKA-1644 > URL: https://issues.apache.org/jira/browse/KAFKA-1644 > Project: Kafka > Issue Type: Bug >Reporter: Anton Karamanov >Assignee: Anton Karamanov > Attachments: > 0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch > > > Unlike all other Kafka API responses {{FetchResponse}} is not a subclass of > RequestOrResponse, which requires handling it as a special case while > processing responses. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-1644) Inherit FetchResponse from RequestOrResponse
[ https://issues.apache.org/jira/browse/KAFKA-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14151423#comment-14151423 ] Anton Karamanov commented on KAFKA-1644: Does this prevent it from being inherited from {{RequestOrResponse}}? From what I see there's no real impact on server-side code and it would be really useful on a client-side. Especially now that [API is completely public (KAFKA-1640)|KAFKA-1640]). My only concern is that {{RequestOrResponse}} trait requires to implement {{writeTo}} method, which wouldn't work for {{FetchResponse}} the same way it does for other {{RequestOrResponse}}s. In the patch I've provided it only serves to write a header of a fetch response. > Inherit FetchResponse from RequestOrResponse > > > Key: KAFKA-1644 > URL: https://issues.apache.org/jira/browse/KAFKA-1644 > Project: Kafka > Issue Type: Bug >Reporter: Anton Karamanov >Assignee: Anton Karamanov > Attachments: > 0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch > > > Unlike all other Kafka API responses {{FetchResponse}} is not a subclass of > RequestOrResponse, which requires handling it as a special case while > processing responses. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-1644) Inherit FetchResponse from RequestOrResponse
[ https://issues.apache.org/jira/browse/KAFKA-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14151136#comment-14151136 ] Jun Rao commented on KAFKA-1644: The reason that FetchResponse is special is that it uses the sendfile api to transfer data from the filechannel to a remote socket, which avoids creating a copy of the data in jvm heap. > Inherit FetchResponse from RequestOrResponse > > > Key: KAFKA-1644 > URL: https://issues.apache.org/jira/browse/KAFKA-1644 > Project: Kafka > Issue Type: Bug >Reporter: Anton Karamanov >Assignee: Anton Karamanov > Attachments: > 0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch > > > Unlike all other Kafka API responses {{FetchResponse}} is not a subclass of > RequestOrResponse, which requires handling it as a special case while > processing responses. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (KAFKA-1644) Inherit FetchResponse from RequestOrResponse
[ https://issues.apache.org/jira/browse/KAFKA-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14143112#comment-14143112 ] Anton Karamanov commented on KAFKA-1644: Here's a [patch|^0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch]. > Inherit FetchResponse from RequestOrResponse > > > Key: KAFKA-1644 > URL: https://issues.apache.org/jira/browse/KAFKA-1644 > Project: Kafka > Issue Type: Bug >Reporter: Anton Karamanov >Assignee: Anton Karamanov > Attachments: > 0001-KAFKA-1644-Inherit-FetchResponse-from-RequestOrRespo.patch > > > Unlike all other Kafka API responses {{FetchResponse}} is not a subclass of > RequestOrResponse, which requires handling it as a special case while > processing responses. -- This message was sent by Atlassian JIRA (v6.3.4#6332)