Hi Anastasia, Thanks for those changes. I eventually figured out that the XYZJsonDataConverter are the new classes from the factoring-out of the Jackson dependency (KAFKA-10384), after which the proposal made much more sense to me. Apologies for being slow on the uptake there.
I can see now that property order in the JSON in the log would be determined by the declared field in the RPC JSON definition. That might not always be ideal since it can be hard to visually parse large unindented JSON, but maybe that could be dealt with once we knew there was a real problem. It's an implementation detail, but I wonder whether constructing a whole tree of JsonNodes might cause performance issues. It would be more work, but the XYZJsonDataConverter could be generated to have a method which took a JsonGenerator, thus avoiding the need to instantiate the nodes just for the purposes of logging. Kind regards, Tom On Fri, Sep 25, 2020 at 7:05 PM Anastasia Vela <av...@confluent.io> wrote: > Hi Tom, > > Thanks for your input! > > 1. I'll add more details for the RequestConvertToJson and > XYZJsonDataConverter classes. Hopefully it will be more clear, but just to > answer your question, RequestConvertToJson does not return a > XYZJsonDataConverter, but rather it returns a JsonNode which will be > serialized. The JsonDataConverter is the new auto-generated schema for each > request/response type that contains the method to return the JsonNode to be > serialized. > > 2. There is no defined order of the properties, rather it's in the order > that it is set in. So if you first set key B, then key A, the properties > would appear with key B first. JsonNodes when serialized does not sort the > keys. > > 3. Yes, serialization is done via Jackson databind. > > Thanks again, > Anastasia > > On Fri, Sep 25, 2020 at 1:15 AM Tom Bentley <tbent...@redhat.com> wrote: > > > Hi Anastasia, > > > > Thanks for the KIP, I can certainly see the benefit of this. I have a few > > questions: > > > > 1. I think it would be helpful to readers to explicitly illustrate the > > RequestConvertToJson and XYZJsonDataConverter classes (e.g. with method > > signatures for one or two methods), because currently it's not clear (to > me > > at least) exactly what's being proposed. Does the RequestConvertToJson > > return a XYZJsonDataConverter? > > > > 2. Does the serialization have a defined order of properties (alphabetic > > perhaps)? My concern here is that properties appearing in order according > > to how they are iterated in a hash map might harm human readability of > the > > logs. > > > > 3. Would the serialization be done via the Jackson databinding? > > > > Many thanks, > > > > Tom > > > > On Thu, Sep 24, 2020 at 11:49 PM Anastasia Vela <av...@confluent.io> > > wrote: > > > > > Hi all, > > > > > > I'd like to discuss KIP-673: > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-673%3A+Emit+JSONs+with+new+auto-generated+schema > > > > > > This is a proposal to change the format of request and response traces > to > > > JSON, which would be easier to load and parse, because the current > format > > > is only JSON-like and not easily parsable. > > > > > > Let me know what you think, > > > Anastasia > > > > > >
