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 >> >> >> >> >> >> >> >> >> >>
