Guys,
I've just uploaded the patch which aligns versionId in getErrorResponse.
(https://issues.apache.org/jira/browse/KAFKA-2195)
Would be great if someone could review it because it's a blocker for
other requests including MetadataRequest which I have to update to V1 for
KIP-4.
Thanks,
Andrii Biletskyi
On Tue, May 19, 2015 at 12:23 AM, Andrii Biletskyi <
andrii.bilets...@stealth.ly> wrote:
> No worries, I think it's hard to foresee all nuances before
> actually implementing/writing code.
> I also have a feeling there will be some other issues
> with requests versioning so I plan to finish end-to-end
> MetadataRequest_V1 to ensure we don't miss anything
> in terms of AbstractRequest/Response API, before uploading
> the respective patch.
>
> Thanks,
> Andrii Biletskyi
>
> On Tue, May 19, 2015 at 12:14 AM, Gwen Shapira <gshap...@cloudera.com>
> wrote:
>
>> Sorry for the confusion regarding errorResponses...
>>
>> On Mon, May 18, 2015 at 9:10 PM, Andrii Biletskyi <
>> andrii.bilets...@stealth.ly> wrote:
>>
>> > Jun,
>> >
>> > Thanks for the explanation. I believe my understanding is close to what
>> > you have written. I see, I still think that this approach is
>> > somewhat limiting
>> > (what if you add field of type int in V1 and then remove another field
>> of
>> > type int in V2 - method overloading for V0 and V2 constructors will not
>> > compile) but in any case we need to follow this approach.
>> >
>> > Ok, then I believe I will have to remove all "error"-constructors which
>> > were
>> > added as part of this sub-task. Instead in getErrorResponse(versionId,
>> > throwable)
>> > I will pattern-match on version and get the right response version
>> > by calling the constructor with the right arguments.
>> >
>> > Also one small issue with this approach. Currently we create
>> > MetadataRequest from a Cluster object. As you remember in KIP-4 we
>> > planned to evolve it to include topic-level configs. We agreed to add
>> > this to Cluster class directly. In this case it will break our pattern -
>> > constructor per version, since the constructor won't be changed (simply
>> > accepting cluster instance in both cases).
>> > What is the preferable solution in this case? I can explicitly add
>> > topicConfigs
>> > param to the signature of the V1 constructor but it seems inconsistent
>> > because
>> > Cluster would already encapsulate topicConfigs at that point.
>> >
>> > Thanks,
>> > Andrii Biletskyi
>> >
>> > On Mon, May 18, 2015 at 8:28 PM, Jun Rao <j...@confluent.io> wrote:
>> >
>> > > Andri,
>> > >
>> > > Let me clarify a bit how things work now. You can see if this fits
>> your
>> > > need or if it can be improved. If you look at OffsetCommitRequest, our
>> > > convention is the following.
>> > >
>> > > 1. The request object can be constructed from a set of required
>> fields.
>> > The
>> > > client typically constructs a request object this way. There will be
>> one
>> > > constructor for each version. The version id is not specified
>> explicitly
>> > > since it's implied by the input parameters. Every time we introduce a
>> new
>> > > version, we will add a new constructor of this form. We will leave the
>> > old
>> > > constructors as they are, but mark them as deprecated. Code compiled
>> with
>> > > the old Kafka jar will still work with the new Kafka jar before we
>> > actually
>> > > remove the deprecated constructors.
>> > >
>> > > 2. The request object can also be constructed from a struct. This is
>> > > typically used by the broker to convert network bytes into a request
>> > > object. Currently, the constructor looks for specific fields in the
>> > struct
>> > > to distinguish which version it corresponds to.
>> > >
>> > > 3. In both cases, the request object always tries to reflect the
>> fields
>> > in
>> > > the latest version. We use the following convention when mapping older
>> > > versions to the latest version in the request object: If a new field
>> is
>> > > added, we try to use a default for the missing field in the old
>> version.
>> > If
>> > > a field is removed, we simply ignore it in the old version.
>> > >
>> > > Thanks,
>> > >
>> > > Jun
>> > >
>> > > On Mon, May 18, 2015 at 8:41 AM, Andrii Biletskyi <
>> > > andrii.bilets...@stealth.ly> wrote:
>> > >
>> > > > Hi all,
>> > > >
>> > > > I started working on it and it seems we are going the wrong way.
>> > > > So it appears we need to distinguish constructors by versions in
>> > > > request/response (so we can set correct schema).
>> > > > Request/Response classes will look like:
>> > > >
>> > > > class SomeRequest extends AbstractRequest {
>> > > > SomeRequest(versionId, <request-specific params >)
>> > > >
>> > > > // for the latest version
>> > > > SomeRequest(<request-specific params>)
>> > > > }
>> > > >
>> > > > Now, what if in SomeRequest_V1 we remove some field from the schema?
>> > > > Well, we can leave constructor signature and simply check
>> > > programmatically
>> > > > if set schema contains given field and if no simply ignore it. Thus
>> > > > mentioned
>> > > > constructor can support V0 & V1. Now, suppose in V2 we add some
>> field -
>> > > > there's nothing we can do, we need to add new parameter and thus add
>> > new
>> > > > constructor:
>> > > > SomeRequest(versionId, <request-specific params for V2>)
>> > > >
>> > > > but it's a bit strange - to introduce constructors which may fail in
>> > > > runtime-only
>> > > > because you used the wrong constructor for your request version.
>> > > > Overall in my opinion such approach depicts we are trying to give
>> > clients
>> > > > factory-like
>> > > > methods but implemented as class constructors...
>> > > >
>> > > > Another thing is about versionId-less constructor (used for the
>> latest
>> > > > version).
>> > > > Again, suppose in V1 we extend schema with additional value, we will
>> > have
>> > > > to change constructor without versionId, because this becomes the
>> > latest
>> > > > version.
>> > > > But would it be considered backward-compatible? Client code that
>> uses
>> > V0
>> > > > and
>> > > > upgrades will not compile in this case.
>> > > >
>> > > > Thoughts?
>> > > >
>> > > > Thanks,
>> > > > Andrii Biletskyi
>> > > >
>> > > >
>> > > >
>> > > >
>> > > > On Fri, May 15, 2015 at 4:31 PM, Andrii Biletskyi <
>> > > > andrii.bilets...@stealth.ly> wrote:
>> > > >
>> > > > > Okay,
>> > > > > I can pick that. I'll create sub-task under KAFKA-2044.
>> > > > >
>> > > > > Thanks,
>> > > > > Andrii Biletskyi
>> > > > >
>> > > > > On Fri, May 15, 2015 at 4:27 PM, Gwen Shapira <
>> gshap...@cloudera.com
>> > >
>> > > > > wrote:
>> > > > >
>> > > > >> Agree that you need version in getErrorResponse too (so you'll
>> get
>> > the
>> > > > >> correct error), which means you'll need to add versionId to
>> > > constructors
>> > > > >> of
>> > > > >> every response object...
>> > > > >>
>> > > > >> You'll want to keep two interfaces, one with version and one with
>> > > > >> CURR_VERSION as default, so you won't need to modify every single
>> > > > call...
>> > > > >>
>> > > > >> On Fri, May 15, 2015 at 4:03 PM, Andrii Biletskyi <
>> > > > >> andrii.bilets...@stealth.ly> wrote:
>> > > > >>
>> > > > >> > Correct, I think we are on the same page.
>> > > > >> > This way we can fix RequestChannel part (where it uses
>> > > > >> > AbstractRequest.getRequest)
>> > > > >> >
>> > > > >> > But would it be okay to add versionId to
>> > > > >> AbstractRequest.getErrorResponse
>> > > > >> > signature too?
>> > > > >> > I'm a bit lost with all those Abstract... objects hierarchy and
>> > not
>> > > > sure
>> > > > >> > whether it's
>> > > > >> > the right solution.
>> > > > >> >
>> > > > >> > Thanks,
>> > > > >> > Andrii Biletskyi
>> > > > >> >
>> > > > >> > On Fri, May 15, 2015 at 3:47 PM, Gwen Shapira <
>> > > gshap...@cloudera.com>
>> > > > >> > wrote:
>> > > > >> >
>> > > > >> > > I agree, we currently don't handle versions correctly when
>> > > > >> de-serializing
>> > > > >> > > into java objects. This will be an isssue for every req/resp
>> we
>> > > move
>> > > > >> to
>> > > > >> > use
>> > > > >> > > the java objects.
>> > > > >> > >
>> > > > >> > > It looks like this requires:
>> > > > >> > > 1. Add versionId parameter to all parse functions in Java
>> > req/resp
>> > > > >> > objects
>> > > > >> > > 2. Modify getRequest to pass it along
>> > > > >> > > 3. Modify RequestChannel to get the version out of the header
>> > and
>> > > > use
>> > > > >> it
>> > > > >> > > when de-serializing the body.
>> > > > >> > >
>> > > > >> > > Did I get that correct? I want to make sure we are talking
>> about
>> > > the
>> > > > >> same
>> > > > >> > > issue.
>> > > > >> > >
>> > > > >> > > Gwen
>> > > > >> > >
>> > > > >> > > On Fri, May 15, 2015 at 1:45 PM, Andrii Biletskyi <
>> > > > >> > > andrii.bilets...@stealth.ly> wrote:
>> > > > >> > >
>> > > > >> > > > Gwen,
>> > > > >> > > >
>> > > > >> > > > I didn't find this in answers above so apologies if this
>> was
>> > > > >> discussed.
>> > > > >> > > > It's about the way we would like to handle request
>> versions.
>> > > > >> > > >
>> > > > >> > > > As I understood from Jun's answer we generally should try
>> > using
>> > > > the
>> > > > >> > same
>> > > > >> > > > java object while evolving the request. I believe the only
>> > > example
>> > > > >> of
>> > > > >> > > > evolved
>> > > > >> > > > request now - OffsetCommitRequest follows this approach.
>> > > > >> > > >
>> > > > >> > > > I'm trying to evolve MetadataRequest to the next version as
>> > part
>> > > > of
>> > > > >> > KIP-4
>> > > > >> > > > and not sure current AbstractRequest api (which is a basis
>> for
>> > > > >> ported
>> > > > >> > to
>> > > > >> > > > java requests)
>> > > > >> > > > is sufficient.
>> > > > >> > > >
>> > > > >> > > > The problem is: in order to deserialize bytes into correct
>> > > correct
>> > > > >> > object
>> > > > >> > > > you need
>> > > > >> > > > to know it's version. Suppose KafkaApi serves
>> > > > OffsetCommitRequestV0
>> > > > >> and
>> > > > >> > > V2
>> > > > >> > > > (current).
>> > > > >> > > > For such cases OffsetCommitRequest class has two
>> constructors:
>> > > > >> > > >
>> > > > >> > > > public static OffsetCommitRequest parse(ByteBuffer buffer,
>> int
>> > > > >> > versionId)
>> > > > >> > > > AND
>> > > > >> > > > public static OffsetCommitRequest parse(ByteBuffer buffer)
>> > > > >> > > >
>> > > > >> > > > The latter one will simply pick the "current" schema
>> version.
>> > > > >> > > > Now AbstractRequest.getRequest which is an entry point for
>> > > > >> > deserializing
>> > > > >> > > > request
>> > > > >> > > > for KafkaApi matches only on RequestHeader.apiKey (and thus
>> > uses
>> > > > the
>> > > > >> > > second
>> > > > >> > > > OffsetCommitRequest constructor) which is not sufficient
>> > because
>> > > > we
>> > > > >> > also
>> > > > >> > > > need
>> > > > >> > > > RequestHeader.apiVersion in case old request version.
>> > > > >> > > >
>> > > > >> > > > The same problem appears in
>> > > > >> AbstractRequest.getErrorResponse(Throwable
>> > > > >> > > e) -
>> > > > >> > > > to construct the right error response object we need to
>> know
>> > to
>> > > > >> which
>> > > > >> > > > apiVersion
>> > > > >> > > > to respond.
>> > > > >> > > >
>> > > > >> > > > I think this can affect other tasks under KAFKA-1927 -
>> > replacing
>> > > > >> > separate
>> > > > >> > > > RQ/RP,
>> > > > >> > > > so maybe it makes sense to decide/fix it once.
>> > > > >> > > >
>> > > > >> > > > Thanks,
>> > > > >> > > > Andrii Bieltskyi
>> > > > >> > > >
>> > > > >> > > >
>> > > > >> > > >
>> > > > >> > > >
>> > > > >> > > >
>> > > > >> > > > On Wed, Mar 25, 2015 at 12:42 AM, Gwen Shapira <
>> > > > >> gshap...@cloudera.com>
>> > > > >> > > > wrote:
>> > > > >> > > >
>> > > > >> > > > > OK, I posted a working patch on KAFKA-2044 and
>> > > > >> > > > > https://reviews.apache.org/r/32459/diff/.
>> > > > >> > > > >
>> > > > >> > > > > There are few decisions there than can be up to
>> discussion
>> > > > >> (factory
>> > > > >> > > > method
>> > > > >> > > > > on AbstractRequestResponse, the new handleErrors in
>> request
>> > > > API),
>> > > > >> but
>> > > > >> > > as
>> > > > >> > > > > far as support for o.a.k.common requests in core goes, it
>> > does
>> > > > >> what
>> > > > >> > it
>> > > > >> > > > > needs to do.
>> > > > >> > > > >
>> > > > >> > > > > Please review!
>> > > > >> > > > >
>> > > > >> > > > > Gwen
>> > > > >> > > > >
>> > > > >> > > > >
>> > > > >> > > > >
>> > > > >> > > > > On Tue, Mar 24, 2015 at 10:59 AM, Gwen Shapira <
>> > > > >> > gshap...@cloudera.com>
>> > > > >> > > > > wrote:
>> > > > >> > > > >
>> > > > >> > > > > > Hi,
>> > > > >> > > > > >
>> > > > >> > > > > > I uploaded a (very) preliminary patch with my idea.
>> > > > >> > > > > >
>> > > > >> > > > > > One thing thats missing:
>> > > > >> > > > > > RequestResponse had handleError method that all
>> requests
>> > > > >> > > implemented,
>> > > > >> > > > > > typically generating appropriate error Response for the
>> > > > request
>> > > > >> and
>> > > > >> > > > > sending
>> > > > >> > > > > > it along. Its used by KafkaApis to handle all protocol
>> > > errors
>> > > > >> for
>> > > > >> > > valid
>> > > > >> > > > > > requests that are not handled elsewhere.
>> > > > >> > > > > > AbstractRequestResponse doesn't have such method.
>> > > > >> > > > > >
>> > > > >> > > > > > I can, of course, add it.
>> > > > >> > > > > > But before I jump into this, I'm wondering if there was
>> > > > another
>> > > > >> > plan
>> > > > >> > > on
>> > > > >> > > > > > handling Api errors.
>> > > > >> > > > > >
>> > > > >> > > > > > Gwen
>> > > > >> > > > > >
>> > > > >> > > > > > On Mon, Mar 23, 2015 at 6:16 PM, Jun Rao <
>> > j...@confluent.io>
>> > > > >> wrote:
>> > > > >> > > > > >
>> > > > >> > > > > >> I think what you are saying is that in
>> RequestChannel, we
>> > > can
>> > > > >> > start
>> > > > >> > > > > >> generating header/body for new request types and leave
>> > > > >> requestObj
>> > > > >> > > > null.
>> > > > >> > > > > >> For
>> > > > >> > > > > >> existing requests, header/body will be null initially.
>> > > > >> Gradually,
>> > > > >> > we
>> > > > >> > > > can
>> > > > >> > > > > >> migrate each type of requests by populating
>> header/body,
>> > > > >> instead
>> > > > >> > of
>> > > > >> > > > > >> requestObj. This makes sense to me since it serves two
>> > > > purposes
>> > > > >> > (1)
>> > > > >> > > > not
>> > > > >> > > > > >> polluting the code base with duplicated
>> request/response
>> > > > >> objects
>> > > > >> > for
>> > > > >> > > > new
>> > > > >> > > > > >> types of requests and (2) allowing the refactoring of
>> > > > existing
>> > > > >> > > > requests
>> > > > >> > > > > to
>> > > > >> > > > > >> be done in smaller pieces.
>> > > > >> > > > > >>
>> > > > >> > > > > >> Could you try that approach and perhaps just migrate
>> one
>> > > > >> existing
>> > > > >> > > > > request
>> > > > >> > > > > >> type (e.g. HeartBeatRequest) as an example? We
>> probably
>> > > need
>> > > > to
>> > > > >> > > rewind
>> > > > >> > > > > the
>> > > > >> > > > > >> buffer after reading the requestId when deserializing
>> the
>> > > > >> header
>> > > > >> > > > (since
>> > > > >> > > > > >> the
>> > > > >> > > > > >> header includes the request id).
>> > > > >> > > > > >>
>> > > > >> > > > > >> Thanks,
>> > > > >> > > > > >>
>> > > > >> > > > > >> Jun
>> > > > >> > > > > >>
>> > > > >> > > > > >> On Mon, Mar 23, 2015 at 4:52 PM, Gwen Shapira <
>> > > > >> > > gshap...@cloudera.com>
>> > > > >> > > > > >> wrote:
>> > > > >> > > > > >>
>> > > > >> > > > > >> > I'm thinking of a different approach, that will not
>> fix
>> > > > >> > > everything,
>> > > > >> > > > > but
>> > > > >> > > > > >> > will allow adding new requests without code
>> duplication
>> > > > (and
>> > > > >> > > > therefore
>> > > > >> > > > > >> > unblock KIP-4):
>> > > > >> > > > > >> >
>> > > > >> > > > > >> > RequestChannel.request currently takes a buffer and
>> > > parses
>> > > > it
>> > > > >> > into
>> > > > >> > > > an
>> > > > >> > > > > >> "old"
>> > > > >> > > > > >> > request object. Since the objects are
>> byte-compatibly,
>> > we
>> > > > >> should
>> > > > >> > > be
>> > > > >> > > > > >> able to
>> > > > >> > > > > >> > parse existing requests into both old and new
>> objects.
>> > > New
>> > > > >> > > requests
>> > > > >> > > > > will
>> > > > >> > > > > >> > only be parsed into new objects.
>> > > > >> > > > > >> >
>> > > > >> > > > > >> > Basically:
>> > > > >> > > > > >> > val requestId = buffer.getShort()
>> > > > >> > > > > >> > if (requestId in keyToNameAndDeserializerMap) {
>> > > > >> > > > > >> > requestObj =
>> > > > >> > RequestKeys.deserializerForKey(requestId)(buffer)
>> > > > >> > > > > >> > header: RequestHeader =
>> RequestHeader.parse(buffer)
>> > > > >> > > > > >> > body: Struct =
>> > > > >> > > > > >> >
>> > > > >> > > > > >>
>> > > > >> > > > >
>> > > > >> > >
>> > > > >>
>> > > >
>> >
>> ProtoUtils.currentRequestSchema(apiKey).read(buffer).asInstanceOf[Struct]
>> > > > >> > > > > >> > } else {
>> > > > >> > > > > >> > requestObj = null
>> > > > >> > > > > >> > header: RequestHeader =
>> RequestHeader.parse(buffer)
>> > > > >> > > > > >> > body: Struct =
>> > > > >> > > > > >> >
>> > > > >> > > > > >>
>> > > > >> > > > >
>> > > > >> > >
>> > > > >>
>> > > >
>> >
>> ProtoUtils.currentRequestSchema(apiKey).read(buffer).asInstanceOf[Struct]
>> > > > >> > > > > >> > }
>> > > > >> > > > > >> >
>> > > > >> > > > > >> > This way existing KafkaApis will keep working as
>> > normal.
>> > > > The
>> > > > >> new
>> > > > >> > > > Apis
>> > > > >> > > > > >> can
>> > > > >> > > > > >> > implement just the new header/body requests.
>> > > > >> > > > > >> > We'll do the same on the send-side:
>> > BoundedByteBufferSend
>> > > > can
>> > > > >> > > have a
>> > > > >> > > > > >> > constructor that takes header/body instead of just a
>> > > > response
>> > > > >> > > > object.
>> > > > >> > > > > >> >
>> > > > >> > > > > >> > Does that make sense?
>> > > > >> > > > > >> >
>> > > > >> > > > > >> > Once we have this in, we can move to:
>> > > > >> > > > > >> > * Adding the missing request/response to the client
>> > code
>> > > > >> > > > > >> > * Replacing requests that can be replaced
>> > > > >> > > > > >> >
>> > > > >> > > > > >> > It will also make life easier by having us review
>> and
>> > > tests
>> > > > >> > > smaller
>> > > > >> > > > > >> chunks
>> > > > >> > > > > >> > of work (the existing patch is *huge* , touches
>> nearly
>> > > > every
>> > > > >> > core
>> > > > >> > > > > >> component
>> > > > >> > > > > >> > and I'm not done yet...)
>> > > > >> > > > > >> >
>> > > > >> > > > > >> > Gwen
>> > > > >> > > > > >> >
>> > > > >> > > > > >> >
>> > > > >> > > > > >> >
>> > > > >> > > > > >> >
>> > > > >> > > > > >> > On Sun, Mar 22, 2015 at 10:24 PM, Jay Kreps <
>> > > > >> > jay.kr...@gmail.com>
>> > > > >> > > > > >> wrote:
>> > > > >> > > > > >> >
>> > > > >> > > > > >> > > Ack, yeah, forgot about that.
>> > > > >> > > > > >> > >
>> > > > >> > > > > >> > > It's not just a difference of wrappers. The server
>> > side
>> > > > >> > actually
>> > > > >> > > > > sends
>> > > > >> > > > > >> > the
>> > > > >> > > > > >> > > bytes lazily using FileChannel.transferTo. We
>> need to
>> > > > make
>> > > > >> it
>> > > > >> > > > > >> possible to
>> > > > >> > > > > >> > > carry over that optimization. In some sense what
>> we
>> > > want
>> > > > >> to be
>> > > > >> > > > able
>> > > > >> > > > > >> to do
>> > > > >> > > > > >> > > is set a value to a Send instead of a ByteBuffer.
>> > > > >> > > > > >> > >
>> > > > >> > > > > >> > > Let me try to add that support to the protocol
>> > > definition
>> > > > >> > stuff,
>> > > > >> > > > > will
>> > > > >> > > > > >> > > probably take me a few days to free up time.
>> > > > >> > > > > >> > >
>> > > > >> > > > > >> > > -Jay
>> > > > >> > > > > >> > >
>> > > > >> > > > > >> > > On Sun, Mar 22, 2015 at 7:44 PM, Gwen Shapira <
>> > > > >> > > > > gshap...@cloudera.com>
>> > > > >> > > > > >> > > wrote:
>> > > > >> > > > > >> > >
>> > > > >> > > > > >> > > > In case anyone is still following this thread, I
>> > > need a
>> > > > >> bit
>> > > > >> > of
>> > > > >> > > > > help
>> > > > >> > > > > >> :)
>> > > > >> > > > > >> > > >
>> > > > >> > > > > >> > > > The old FetchResponse.PartitionData included a
>> > > > MessageSet
>> > > > >> > > > object.
>> > > > >> > > > > >> > > > The new FetchResponse.PartitionData includes a
>> > > > >> ByteBuffer.
>> > > > >> > > > > >> > > >
>> > > > >> > > > > >> > > > However, when we read from logs, we return a
>> > > > MessageSet,
>> > > > >> and
>> > > > >> > > as
>> > > > >> > > > > far
>> > > > >> > > > > >> as
>> > > > >> > > > > >> > I
>> > > > >> > > > > >> > > > can see, these can't be converted to ByteBuffers
>> > (at
>> > > > >> least
>> > > > >> > not
>> > > > >> > > > > >> without
>> > > > >> > > > > >> > > > copying their data).
>> > > > >> > > > > >> > > >
>> > > > >> > > > > >> > > > Did anyone consider how to reconcile the
>> > MessageSets
>> > > > with
>> > > > >> > the
>> > > > >> > > > new
>> > > > >> > > > > >> > > > FetchResponse objects?
>> > > > >> > > > > >> > > >
>> > > > >> > > > > >> > > > Gwen
>> > > > >> > > > > >> > > >
>> > > > >> > > > > >> > > >
>> > > > >> > > > > >> > > > On Sat, Mar 21, 2015 at 6:54 PM, Gwen Shapira <
>> > > > >> > > > > >> gshap...@cloudera.com>
>> > > > >> > > > > >> > > > wrote:
>> > > > >> > > > > >> > > >
>> > > > >> > > > > >> > > > > Note: I'm also treating ZkUtils as if it was a
>> > > public
>> > > > >> API
>> > > > >> > > > (i.e.
>> > > > >> > > > > >> > > > converting
>> > > > >> > > > > >> > > > > objects that are returned into o.a.k.common
>> > > > equivalents
>> > > > >> > but
>> > > > >> > > > not
>> > > > >> > > > > >> > > changing
>> > > > >> > > > > >> > > > > ZkUtils itself).
>> > > > >> > > > > >> > > > > I know its not public, but I suspect I'm not
>> the
>> > > only
>> > > > >> > > > developer
>> > > > >> > > > > >> here
>> > > > >> > > > > >> > > who
>> > > > >> > > > > >> > > > > has tons of external code that uses it.
>> > > > >> > > > > >> > > > >
>> > > > >> > > > > >> > > > > Gwen
>> > > > >> > > > > >> > > > >
>> > > > >> > > > > >> > > > > On Wed, Mar 18, 2015 at 5:48 PM, Gwen Shapira
>> <
>> > > > >> > > > > >> gshap...@cloudera.com
>> > > > >> > > > > >> > >
>> > > > >> > > > > >> > > > > wrote:
>> > > > >> > > > > >> > > > >
>> > > > >> > > > > >> > > > >> We can't rip them out completely,
>> unfortunately
>> > -
>> > > > the
>> > > > >> > > > > >> SimpleConsumer
>> > > > >> > > > > >> > > > uses
>> > > > >> > > > > >> > > > >> them.
>> > > > >> > > > > >> > > > >>
>> > > > >> > > > > >> > > > >> So we'll need conversion at some point. I'll
>> try
>> > > to
>> > > > >> make
>> > > > >> > > the
>> > > > >> > > > > >> > > > >> conversion point "just before hitting a
>> public
>> > API
>> > > > >> that
>> > > > >> > we
>> > > > >> > > > > can't
>> > > > >> > > > > >> > > > >> modify", and hopefully it won't look too
>> > > arbitrary.
>> > > > >> > > > > >> > > > >>
>> > > > >> > > > > >> > > > >>
>> > > > >> > > > > >> > > > >>
>> > > > >> > > > > >> > > > >> On Wed, Mar 18, 2015 at 5:24 PM, Jay Kreps <
>> > > > >> > > > > jay.kr...@gmail.com>
>> > > > >> > > > > >> > > wrote:
>> > > > >> > > > > >> > > > >> > I think either approach is okay in the
>> short
>> > > term.
>> > > > >> > > However
>> > > > >> > > > > our
>> > > > >> > > > > >> > goal
>> > > > >> > > > > >> > > > >> should
>> > > > >> > > > > >> > > > >> > be to eventually get rid of that duplicate
>> > code,
>> > > > so
>> > > > >> if
>> > > > >> > > you
>> > > > >> > > > > are
>> > > > >> > > > > >> up
>> > > > >> > > > > >> > > for
>> > > > >> > > > > >> > > > >> just
>> > > > >> > > > > >> > > > >> > ripping and cutting that may get us there
>> > > sooner.
>> > > > >> > > > > >> > > > >> >
>> > > > >> > > > > >> > > > >> > -Jay
>> > > > >> > > > > >> > > > >> >
>> > > > >> > > > > >> > > > >> > On Wed, Mar 18, 2015 at 5:19 PM, Gwen
>> Shapira
>> > <
>> > > > >> > > > > >> > > gshap...@cloudera.com>
>> > > > >> > > > > >> > > > >> wrote:
>> > > > >> > > > > >> > > > >> >
>> > > > >> > > > > >> > > > >> >> Thanks!
>> > > > >> > > > > >> > > > >> >>
>> > > > >> > > > > >> > > > >> >> Another clarification:
>> > > > >> > > > > >> > > > >> >> The Common request/responses use slightly
>> > > > different
>> > > > >> > > > > >> > infrastructure
>> > > > >> > > > > >> > > > >> >> objects: Node instead of Broker,
>> > TopicPartition
>> > > > >> > instead
>> > > > >> > > of
>> > > > >> > > > > >> > > > >> >> TopicAndPartition and few more.
>> > > > >> > > > > >> > > > >> >>
>> > > > >> > > > > >> > > > >> >> I can write utilities to convert Node to
>> > Broker
>> > > > to
>> > > > >> > > > minimize
>> > > > >> > > > > >> the
>> > > > >> > > > > >> > > scope
>> > > > >> > > > > >> > > > >> >> of the change.
>> > > > >> > > > > >> > > > >> >> Or I can start replacing Brokers with
>> Nodes
>> > > > across
>> > > > >> the
>> > > > >> > > > > board.
>> > > > >> > > > > >> > > > >> >>
>> > > > >> > > > > >> > > > >> >> I'm currently taking the second approach -
>> > i.e,
>> > > > if
>> > > > >> > > > > >> > MetadataRequest
>> > > > >> > > > > >> > > is
>> > > > >> > > > > >> > > > >> >> now returning Node, I'm changing the
>> entire
>> > > line
>> > > > of
>> > > > >> > > > > >> dependencies
>> > > > >> > > > > >> > to
>> > > > >> > > > > >> > > > >> >> use Nodes instead of broker.
>> > > > >> > > > > >> > > > >> >>
>> > > > >> > > > > >> > > > >> >> Is this acceptable, or do we want to take
>> a
>> > > more
>> > > > >> > minimal
>> > > > >> > > > > >> approach
>> > > > >> > > > > >> > > for
>> > > > >> > > > > >> > > > >> >> this patch and do a larger replacement as
>> a
>> > > > follow
>> > > > >> up?
>> > > > >> > > > > >> > > > >> >>
>> > > > >> > > > > >> > > > >> >> Gwen
>> > > > >> > > > > >> > > > >> >>
>> > > > >> > > > > >> > > > >> >>
>> > > > >> > > > > >> > > > >> >>
>> > > > >> > > > > >> > > > >> >>
>> > > > >> > > > > >> > > > >> >> On Wed, Mar 18, 2015 at 3:32 PM, Jay
>> Kreps <
>> > > > >> > > > > >> jay.kr...@gmail.com>
>> > > > >> > > > > >> > > > >> wrote:
>> > > > >> > > > > >> > > > >> >> > Great.
>> > > > >> > > > > >> > > > >> >> >
>> > > > >> > > > > >> > > > >> >> > For (3) yeah I think we should just
>> think
>> > > > through
>> > > > >> > the
>> > > > >> > > > > >> > end-to-end
>> > > > >> > > > > >> > > > >> pattern
>> > > > >> > > > > >> > > > >> >> > for these versioned requests since it
>> seems
>> > > > like
>> > > > >> we
>> > > > >> > > will
>> > > > >> > > > > >> have a
>> > > > >> > > > > >> > > > >> number of
>> > > > >> > > > > >> > > > >> >> > them. The serialization code used as you
>> > > > >> described
>> > > > >> > > gets
>> > > > >> > > > us
>> > > > >> > > > > >> to
>> > > > >> > > > > >> > the
>> > > > >> > > > > >> > > > >> right
>> > > > >> > > > > >> > > > >> >> > Struct which the user would then wrap in
>> > > > >> something
>> > > > >> > > like
>> > > > >> > > > > >> > > > >> ProduceRequest.
>> > > > >> > > > > >> > > > >> >> > Presumably there would just be one
>> > > > ProduceRequest
>> > > > >> > that
>> > > > >> > > > > would
>> > > > >> > > > > >> > > > >> internally
>> > > > >> > > > > >> > > > >> >> > fill in things like null or otherwise
>> adapt
>> > > the
>> > > > >> > struct
>> > > > >> > > > to
>> > > > >> > > > > a
>> > > > >> > > > > >> > > usable
>> > > > >> > > > > >> > > > >> >> object.
>> > > > >> > > > > >> > > > >> >> > On the response side we would have the
>> > > version
>> > > > >> from
>> > > > >> > > the
>> > > > >> > > > > >> request
>> > > > >> > > > > >> > > to
>> > > > >> > > > > >> > > > >> use
>> > > > >> > > > > >> > > > >> >> for
>> > > > >> > > > > >> > > > >> >> > correct versioning. On question is
>> whether
>> > > this
>> > > > >> is
>> > > > >> > > > enough
>> > > > >> > > > > or
>> > > > >> > > > > >> > > > whether
>> > > > >> > > > > >> > > > >> we
>> > > > >> > > > > >> > > > >> >> > need to have switches in KafkaApis to do
>> > > things
>> > > > >> like
>> > > > >> > > > > >> > > > >> >> > if(produceRequest.version == 3)
>> > > > >> > > > > >> > > > >> >> > // do something
>> > > > >> > > > > >> > > > >> >> > else
>> > > > >> > > > > >> > > > >> >> > // do something else
>> > > > >> > > > > >> > > > >> >> >
>> > > > >> > > > > >> > > > >> >> > Basically it would be good to be able to
>> > > write
>> > > > a
>> > > > >> > quick
>> > > > >> > > > > wiki
>> > > > >> > > > > >> > that
>> > > > >> > > > > >> > > > was
>> > > > >> > > > > >> > > > >> like
>> > > > >> > > > > >> > > > >> >> > "how to add or modify a kafka api" that
>> > > > explained
>> > > > >> > the
>> > > > >> > > > > right
>> > > > >> > > > > >> way
>> > > > >> > > > > >> > > to
>> > > > >> > > > > >> > > > >> do all
>> > > > >> > > > > >> > > > >> >> > this.
>> > > > >> > > > > >> > > > >> >> >
>> > > > >> > > > > >> > > > >> >> > I don't think any of this necessarily
>> > blocks
>> > > > this
>> > > > >> > > ticket
>> > > > >> > > > > >> since
>> > > > >> > > > > >> > at
>> > > > >> > > > > >> > > > the
>> > > > >> > > > > >> > > > >> >> > moment we don't have tons of versions of
>> > > > requests
>> > > > >> > out
>> > > > >> > > > > there.
>> > > > >> > > > > >> > > > >> >> >
>> > > > >> > > > > >> > > > >> >> > -Jay
>> > > > >> > > > > >> > > > >> >> >
>> > > > >> > > > > >> > > > >> >> > On Wed, Mar 18, 2015 at 2:50 PM, Gwen
>> > > Shapira <
>> > > > >> > > > > >> > > > gshap...@cloudera.com
>> > > > >> > > > > >> > > > >> >
>> > > > >> > > > > >> > > > >> >> wrote:
>> > > > >> > > > > >> > > > >> >> >
>> > > > >> > > > > >> > > > >> >> >> See inline responses:
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >> On Wed, Mar 18, 2015 at 2:26 PM, Jay
>> > Kreps <
>> > > > >> > > > > >> > jay.kr...@gmail.com
>> > > > >> > > > > >> > > >
>> > > > >> > > > > >> > > > >> wrote:
>> > > > >> > > > > >> > > > >> >> >> > Hey Gwen,
>> > > > >> > > > > >> > > > >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> > This makes sense to me.
>> > > > >> > > > > >> > > > >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> > A couple of thoughts, mostly
>> confirming
>> > > what
>> > > > >> you
>> > > > >> > > > said I
>> > > > >> > > > > >> > think:
>> > > > >> > > > > >> > > > >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> > 1. Ideally we would move
>> completely
>> > > over
>> > > > to
>> > > > >> > the
>> > > > >> > > > new
>> > > > >> > > > > >> style
>> > > > >> > > > > >> > > of
>> > > > >> > > > > >> > > > >> >> request
>> > > > >> > > > > >> > > > >> >> >> > definition for server-side
>> > processing,
>> > > > even
>> > > > >> > for
>> > > > >> > > > the
>> > > > >> > > > > >> > > internal
>> > > > >> > > > > >> > > > >> >> >> requests. This
>> > > > >> > > > > >> > > > >> >> >> > way all requests would have the
>> same
>> > > > >> > header/body
>> > > > >> > > > > >> struct
>> > > > >> > > > > >> > > > stuff.
>> > > > >> > > > > >> > > > >> As
>> > > > >> > > > > >> > > > >> >> you
>> > > > >> > > > > >> > > > >> >> >> say
>> > > > >> > > > > >> > > > >> >> >> > for the internal requests we can
>> just
>> > > > >> delete
>> > > > >> > the
>> > > > >> > > > > scala
>> > > > >> > > > > >> > > code.
>> > > > >> > > > > >> > > > >> For
>> > > > >> > > > > >> > > > >> >> the
>> > > > >> > > > > >> > > > >> >> >> old
>> > > > >> > > > > >> > > > >> >> >> > clients they will continue to use
>> > their
>> > > > old
>> > > > >> > > > request
>> > > > >> > > > > >> > > > definitions
>> > > > >> > > > > >> > > > >> >> until
>> > > > >> > > > > >> > > > >> >> >> we
>> > > > >> > > > > >> > > > >> >> >> > eol them. I would propose that new
>> > > > changes
>> > > > >> > will
>> > > > >> > > go
>> > > > >> > > > > >> only
>> > > > >> > > > > >> > > into
>> > > > >> > > > > >> > > > >> the
>> > > > >> > > > > >> > > > >> >> new
>> > > > >> > > > > >> > > > >> >> >> > request/response objects and the
>> old
>> > > > scala
>> > > > >> > ones
>> > > > >> > > > will
>> > > > >> > > > > >> be
>> > > > >> > > > > >> > > > >> permanently
>> > > > >> > > > > >> > > > >> >> >> stuck
>> > > > >> > > > > >> > > > >> >> >> > on their current version until
>> > > > >> discontinued.
>> > > > >> > So
>> > > > >> > > > > after
>> > > > >> > > > > >> > this
>> > > > >> > > > > >> > > > >> change
>> > > > >> > > > > >> > > > >> >> >> that old
>> > > > >> > > > > >> > > > >> >> >> > scala code could be considered
>> > frozen.
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >> SimpleConsumer is obviously stuck with
>> the
>> > > old
>> > > > >> > > > > >> > request/response.
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >> The Producers can be converted to the
>> > common
>> > > > >> > > > > >> request/response
>> > > > >> > > > > >> > > > >> without
>> > > > >> > > > > >> > > > >> >> >> breaking compatibility.
>> > > > >> > > > > >> > > > >> >> >> I think we should do this (even though
>> it
>> > > > >> requires
>> > > > >> > > > > fiddling
>> > > > >> > > > > >> > with
>> > > > >> > > > > >> > > > >> >> >> additional network serialization code),
>> > just
>> > > > so
>> > > > >> we
>> > > > >> > > can
>> > > > >> > > > > >> throw
>> > > > >> > > > > >> > the
>> > > > >> > > > > >> > > > old
>> > > > >> > > > > >> > > > >> >> >> ProduceRequest away.
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >> Does that make sense?
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >> > 2. I think it would be reasonable
>> to
>> > > keep
>> > > > >> all
>> > > > >> > > the
>> > > > >> > > > > >> > requests
>> > > > >> > > > > >> > > > >> under
>> > > > >> > > > > >> > > > >> >> >> common,
>> > > > >> > > > > >> > > > >> >> >> > even though as you point out
>> there is
>> > > > >> > currently
>> > > > >> > > no
>> > > > >> > > > > use
>> > > > >> > > > > >> > for
>> > > > >> > > > > >> > > > >> some of
>> > > > >> > > > > >> > > > >> >> >> them
>> > > > >> > > > > >> > > > >> >> >> > beyond broker-to-broker
>> communication
>> > > at
>> > > > >> the
>> > > > >> > > > moment.
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >> Yep.
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >> > 3. We should think a little about
>> how
>> > > > >> > versioning
>> > > > >> > > > > will
>> > > > >> > > > > >> > work.
>> > > > >> > > > > >> > > > >> Making
>> > > > >> > > > > >> > > > >> >> >> this
>> > > > >> > > > > >> > > > >> >> >> > convenient on the server side is
>> an
>> > > > >> important
>> > > > >> > > goal
>> > > > >> > > > > for
>> > > > >> > > > > >> > the
>> > > > >> > > > > >> > > > new
>> > > > >> > > > > >> > > > >> >> style
>> > > > >> > > > > >> > > > >> >> >> of
>> > > > >> > > > > >> > > > >> >> >> > request definition. At the
>> > > serialization
>> > > > >> level
>> > > > >> > > we
>> > > > >> > > > > now
>> > > > >> > > > > >> > > handle
>> > > > >> > > > > >> > > > >> >> >> versioning but
>> > > > >> > > > > >> > > > >> >> >> > the question we should discuss and
>> > work
>> > > > >> out is
>> > > > >> > > how
>> > > > >> > > > > >> this
>> > > > >> > > > > >> > > will
>> > > > >> > > > > >> > > > >> map to
>> > > > >> > > > > >> > > > >> >> >> the
>> > > > >> > > > > >> > > > >> >> >> > request objects (which I assume
>> will
>> > > > remain
>> > > > >> > > > > >> unversioned).
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >> The way I see it working (I just
>> started
>> > on
>> > > > >> this,
>> > > > >> > so
>> > > > >> > > I
>> > > > >> > > > > may
>> > > > >> > > > > >> > have
>> > > > >> > > > > >> > > > >> gaps):
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >> * Request header contains the version
>> > > > >> > > > > >> > > > >> >> >> * When we read the request, we use
>> > > > >> > > > > ProtoUtils.requestSchema
>> > > > >> > > > > >> > > which
>> > > > >> > > > > >> > > > >> >> >> takes version as a parameter and is
>> > > > responsible
>> > > > >> to
>> > > > >> > > give
>> > > > >> > > > > us
>> > > > >> > > > > >> the
>> > > > >> > > > > >> > > > right
>> > > > >> > > > > >> > > > >> >> >> Schema, which we use to read the buffer
>> > and
>> > > > get
>> > > > >> the
>> > > > >> > > > > correct
>> > > > >> > > > > >> > > > struct.
>> > > > >> > > > > >> > > > >> >> >> * KafkaApis handlers have the header,
>> so
>> > > they
>> > > > >> can
>> > > > >> > use
>> > > > >> > > > it
>> > > > >> > > > > to
>> > > > >> > > > > >> > > access
>> > > > >> > > > > >> > > > >> the
>> > > > >> > > > > >> > > > >> >> >> correct fields, build the correct
>> > response,
>> > > > etc.
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >> Does that sound about right?
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >> > 4. Ideally after this refactoring
>> the
>> > > > >> network
>> > > > >> > > > > package
>> > > > >> > > > > >> > > should
>> > > > >> > > > > >> > > > >> not be
>> > > > >> > > > > >> > > > >> >> >> > dependent on the individual
>> request
>> > > > >> objects.
>> > > > >> > The
>> > > > >> > > > > >> > intention
>> > > > >> > > > > >> > > is
>> > > > >> > > > > >> > > > >> that
>> > > > >> > > > > >> > > > >> >> >> stuff in
>> > > > >> > > > > >> > > > >> >> >> > kafka.network is meant to be
>> generic
>> > > > >> network
>> > > > >> > > > > >> > infrastructure
>> > > > >> > > > > >> > > > >> that
>> > > > >> > > > > >> > > > >> >> >> doesn't
>> > > > >> > > > > >> > > > >> >> >> > know about the particular
>> > fetch/produce
>> > > > >> apis
>> > > > >> > we
>> > > > >> > > > have
>> > > > >> > > > > >> > > > >> implemented on
>> > > > >> > > > > >> > > > >> >> >> top.
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >> I'll make a note to validate that this
>> is
>> > > the
>> > > > >> case.
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> > -Jay
>> > > > >> > > > > >> > > > >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> > On Wed, Mar 18, 2015 at 11:11 AM,
>> Gwen
>> > > > >> Shapira <
>> > > > >> > > > > >> > > > >> gshap...@cloudera.com
>> > > > >> > > > > >> > > > >> >> >
>> > > > >> > > > > >> > > > >> >> >> > wrote:
>> > > > >> > > > > >> > > > >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> >> Hi Jun,
>> > > > >> > > > > >> > > > >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >> I was taking a slightly different
>> > > approach.
>> > > > >> Let
>> > > > >> > me
>> > > > >> > > > > know
>> > > > >> > > > > >> if
>> > > > >> > > > > >> > it
>> > > > >> > > > > >> > > > >> makes
>> > > > >> > > > > >> > > > >> >> >> >> sense to you:
>> > > > >> > > > > >> > > > >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >> 1. Get the bytes from network (kinda
>> > > > >> > > unavoidable...)
>> > > > >> > > > > >> > > > >> >> >> >> 2. Modify RequestChannel.Request to
>> > > contain
>> > > > >> > header
>> > > > >> > > > and
>> > > > >> > > > > >> body
>> > > > >> > > > > >> > > > >> (instead
>> > > > >> > > > > >> > > > >> >> >> >> of a single object)
>> > > > >> > > > > >> > > > >> >> >> >> 3. Create the head and body from
>> bytes
>> > as
>> > > > >> > follow:
>> > > > >> > > > > >> > > > >> >> >> >> val header: RequestHeader =
>> > > > >> > > > > >> RequestHeader.parse(buffer)
>> > > > >> > > > > >> > > > >> >> >> >> val apiKey: Int = header.apiKey
>> > > > >> > > > > >> > > > >> >> >> >> val body: Struct =
>> > > > >> > > > > >> > > > >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >>
>> > > > >> > > > > >> > > > >>
>> > > > >> > > > > >> > > >
>> > > > >> > > > > >> >
>> > > > >> > > > > >>
>> > > > >> > > > >
>> > > > >> > >
>> > > > >>
>> > > >
>> >
>> ProtoUtils.currentRequestSchema(apiKey).read(buffer).asInstanceOf[Struct]
>> > > > >> > > > > >> > > > >> >> >> >> 4. KafkaAPIs will continue getting
>> > > > >> > > > > >> RequestChannel.Request,
>> > > > >> > > > > >> > > but
>> > > > >> > > > > >> > > > >> will
>> > > > >> > > > > >> > > > >> >> >> >> now have access to body and header
>> > > > >> separately.
>> > > > >> > > > > >> > > > >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >> I agree that I need a
>> Request/Response
>> > > > >> objects
>> > > > >> > > that
>> > > > >> > > > > >> contain
>> > > > >> > > > > >> > > > only
>> > > > >> > > > > >> > > > >> the
>> > > > >> > > > > >> > > > >> >> >> >> body for all requests objects.
>> > > > >> > > > > >> > > > >> >> >> >> I'm thinking of implementing them in
>> > > > >> > > > > >> o.a.k.Common.Requests
>> > > > >> > > > > >> > in
>> > > > >> > > > > >> > > > >> Java
>> > > > >> > > > > >> > > > >> >> for
>> > > > >> > > > > >> > > > >> >> >> >> consistency.
>> > > > >> > > > > >> > > > >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >> When we are discussing the
>> > > > requests/responses
>> > > > >> > used
>> > > > >> > > > in
>> > > > >> > > > > >> > > > >> SimpleConsumer,
>> > > > >> > > > > >> > > > >> >> >> >> we mean everything used in javaapi,
>> > > right?
>> > > > >> > > > > >> > > > >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >> Gwen
>> > > > >> > > > > >> > > > >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >> On Wed, Mar 18, 2015 at 9:55 AM, Jun
>> > Rao
>> > > <
>> > > > >> > > > > >> j...@confluent.io
>> > > > >> > > > > >> > >
>> > > > >> > > > > >> > > > >> wrote:
>> > > > >> > > > > >> > > > >> >> >> >> > Hi, Gwen,
>> > > > >> > > > > >> > > > >> >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> >> > I was thinking that we will be
>> doing
>> > > the
>> > > > >> > > following
>> > > > >> > > > > in
>> > > > >> > > > > >> > > > >> KAFKA-1927.
>> > > > >> > > > > >> > > > >> >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> >> > 1. Get the bytes from network.
>> > > > >> > > > > >> > > > >> >> >> >> > 2. Use a new generic approach to
>> > > convert
>> > > > >> bytes
>> > > > >> > > > into
>> > > > >> > > > > >> > request
>> > > > >> > > > > >> > > > >> >> objects.
>> > > > >> > > > > >> > > > >> >> >> >> > 2.1 Read the fixed request header
>> > > (using
>> > > > >> the
>> > > > >> > > util
>> > > > >> > > > in
>> > > > >> > > > > >> > > client).
>> > > > >> > > > > >> > > > >> >> >> >> > 2.2 Based on the request id in the
>> > > > header,
>> > > > >> > > > > deserialize
>> > > > >> > > > > >> > the
>> > > > >> > > > > >> > > > >> rest of
>> > > > >> > > > > >> > > > >> >> the
>> > > > >> > > > > >> > > > >> >> >> >> > bytes into a request specific
>> object
>> > > > (using
>> > > > >> > the
>> > > > >> > > > new
>> > > > >> > > > > >> java
>> > > > >> > > > > >> > > > >> objects).
>> > > > >> > > > > >> > > > >> >> >> >> > 3. We will then be passing a
>> header
>> > and
>> > > > an
>> > > > >> > > > > >> > > > >> AbstractRequestResponse
>> > > > >> > > > > >> > > > >> >> to
>> > > > >> > > > > >> > > > >> >> >> >> > KafkaApis.
>> > > > >> > > > > >> > > > >> >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> >> > In order to do that, we will need
>> to
>> > > > create
>> > > > >> > > > similar
>> > > > >> > > > > >> > > > >> >> request/response
>> > > > >> > > > > >> > > > >> >> >> >> > objects for internal requests
>> such as
>> > > > >> > > StopReplica,
>> > > > >> > > > > >> > > > >> LeaderAndIsr,
>> > > > >> > > > > >> > > > >> >> >> >> > UpdateMetadata,
>> ControlledShutdown.
>> > Not
>> > > > >> sure
>> > > > >> > > > whether
>> > > > >> > > > > >> they
>> > > > >> > > > > >> > > > >> should be
>> > > > >> > > > > >> > > > >> >> >> >> written
>> > > > >> > > > > >> > > > >> >> >> >> > in java or scala, but perhaps they
>> > > should
>> > > > >> be
>> > > > >> > > only
>> > > > >> > > > in
>> > > > >> > > > > >> the
>> > > > >> > > > > >> > > core
>> > > > >> > > > > >> > > > >> >> project.
>> > > > >> > > > > >> > > > >> >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> >> > Also note, there are some scala
>> > > > >> > > requests/responses
>> > > > >> > > > > >> used
>> > > > >> > > > > >> > > > >> directly in
>> > > > >> > > > > >> > > > >> >> >> >> > SimpleConsumer. Since that's our
>> > public
>> > > > >> api,
>> > > > >> > we
>> > > > >> > > > > can't
>> > > > >> > > > > >> > > remove
>> > > > >> > > > > >> > > > >> those
>> > > > >> > > > > >> > > > >> >> >> scala
>> > > > >> > > > > >> > > > >> >> >> >> > objects until the old consumer is
>> > > phased
>> > > > >> out.
>> > > > >> > We
>> > > > >> > > > can
>> > > > >> > > > > >> > remove
>> > > > >> > > > > >> > > > the
>> > > > >> > > > > >> > > > >> >> rest
>> > > > >> > > > > >> > > > >> >> >> of
>> > > > >> > > > > >> > > > >> >> >> >> the
>> > > > >> > > > > >> > > > >> >> >> >> > scala request objects.
>> > > > >> > > > > >> > > > >> >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> >> > Thanks,
>> > > > >> > > > > >> > > > >> >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> >> > Jun
>> > > > >> > > > > >> > > > >> >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> >> > On Tue, Mar 17, 2015 at 6:08 PM,
>> Gwen
>> > > > >> Shapira
>> > > > >> > <
>> > > > >> > > > > >> > > > >> >> gshap...@cloudera.com>
>> > > > >> > > > > >> > > > >> >> >> >> wrote:
>> > > > >> > > > > >> > > > >> >> >> >> >
>> > > > >> > > > > >> > > > >> >> >> >> >> Hi,
>> > > > >> > > > > >> > > > >> >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >> >> I'm starting this thread for the
>> > > various
>> > > > >> > > > questions
>> > > > >> > > > > I
>> > > > >> > > > > >> run
>> > > > >> > > > > >> > > > into
>> > > > >> > > > > >> > > > >> >> while
>> > > > >> > > > > >> > > > >> >> >> >> >> refactoring the server to use
>> client
>> > > > >> requests
>> > > > >> > > and
>> > > > >> > > > > >> > > responses.
>> > > > >> > > > > >> > > > >> >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >> >> Help is appreciated :)
>> > > > >> > > > > >> > > > >> >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >> >> First question: LEADER_AND_ISR
>> > request
>> > > > and
>> > > > >> > > > > >> STOP_REPLICA
>> > > > >> > > > > >> > > > >> request
>> > > > >> > > > > >> > > > >> >> are
>> > > > >> > > > > >> > > > >> >> >> >> >> unimplemented in the client.
>> > > > >> > > > > >> > > > >> >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >> >> Do we want to implement them as
>> part
>> > > of
>> > > > >> this
>> > > > >> > > > > >> > refactoring?
>> > > > >> > > > > >> > > > >> >> >> >> >> Or should we continue using the
>> > scala
>> > > > >> > > > > implementation
>> > > > >> > > > > >> for
>> > > > >> > > > > >> > > > >> those?
>> > > > >> > > > > >> > > > >> >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >> >> Gwen
>> > > > >> > > > > >> > > > >> >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >> >>
>> > > > >> > > > > >> > > > >> >> >>
>> > > > >> > > > > >> > > > >> >>
>> > > > >> > > > > >> > > > >>
>> > > > >> > > > > >> > > > >
>> > > > >> > > > > >> > > > >
>> > > > >> > > > > >> > > >
>> > > > >> > > > > >> > >
>> > > > >> > > > > >> >
>> > > > >> > > > > >>
>> > > > >> > > > > >
>> > > > >> > > > > >
>> > > > >> > > > >
>> > > > >> > > >
>> > > > >> > >
>> > > > >> >
>> > > > >>
>> > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
>
