> On Jan. 28, 2015, 8:49 p.m., Rafael Schloming wrote: > > proton-j/src/main/java/org/apache/qpid/proton/amqp/transport2/Flow.java, > > line 141 > > <https://reviews.apache.org/r/30379/diff/1/?file=839118#file839118line141> > > > > do we want to specify the types for the Map here?
We should IMO. The current code does not. I was thinking <String, Object>. > On Jan. 28, 2015, 8:49 p.m., Rafael Schloming wrote: > > proton-j/src/main/java/org/apache/qpid/proton/codec2/TransportTypesDecoder.java, > > line 32 > > <https://reviews.apache.org/r/30379/diff/1/?file=839119#file839119line32> > > > > I'm not quite following how this part works. How do you know that the > > data you have is a flow? Or is this a callback that is made when you've > > detected a flow in the decoding stream? If the latter is the case, what is > > actually invoking this? Yes this is called when we detect it's a "flow" performative in the decoding stream. I didn't put together the part that was going to call this method yet. I will do so shortly > On Jan. 28, 2015, 8:49 p.m., Rafael Schloming wrote: > > proton-j/src/main/java/org/apache/qpid/proton/codec2/TransportTypesEncoder.java, > > line 32 > > <https://reviews.apache.org/r/30379/diff/1/?file=839120#file839120line32> > > > > It might be worth introducing an Encodable interface with > > Encodable.encode(Encoder) and Encodable.decode(Decoder). That way you could > > keep the encode and decode logic together in the performative itself. I > > think having the encode/decode logic together would be good so that you > > could easily spot if they are asymmetric. It would also ease code > > generation if you end up going that route since you just need to generate > > the one class. > > > > I think it would also give you a good way to supply the missing piece > > of your decode picture that I mentioned above, since you could just have a > > general purpose decoder that holds a map from a descriptor to a factory. > > Then in your decoder when you get the descriptor callback you can lookup > > and construct the appropriate Encodable and delegate to it. I was initially hesitant to put any codec logic in the POJO version of the performatives. But I think the approach you propose is a good compromise and would come in handy for code generation and building a general purpose decoder. I will be posting another patch tonight with changes. - rajith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30379/#review70071 ----------------------------------------------------------- On Jan. 28, 2015, 8:04 p.m., rajith attapattu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30379/ > ----------------------------------------------------------- > > (Updated Jan. 28, 2015, 8:04 p.m.) > > > Review request for qpid and Rafael Schloming. > > > Repository: qpid-proton-git > > > Description > ------- > > Skeleton code for a possible approach for codec improvements in proton-j. > > > Diffs > ----- > > proton-j/src/main/java/org/apache/qpid/proton/amqp/transport2/Flow.java > PRE-CREATION > > proton-j/src/main/java/org/apache/qpid/proton/codec2/TransportTypesDecoder.java > PRE-CREATION > > proton-j/src/main/java/org/apache/qpid/proton/codec2/TransportTypesEncoder.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/30379/diff/ > > > Testing > ------- > > > Thanks, > > rajith attapattu > >
