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