Is this going to become a dependency for core and then transitively for the old clients? The current json library is definitely not great, but it does parse json and it's not used in any context where performance is a concern.
Because the older clients aren't well modularized, adding core dependencies sucks these up into every user of the clients. This particularly becomes a problem with common libraries since it will turn out we require version X but other code in the same app requires version Y. The new clients fix this issue but not everyone is using them yet. If there is a pressing need maybe we should just do it and people who have problems can just hack their build to exclude the dependency (since the client code won't need it). If not it might be better just to leave it for a bit until we have at least get a couple releases with both the new producer and the new consumer. -Jay On Mon, Jul 13, 2015 at 2:05 PM, Ismael Juma <ism...@juma.me.uk> wrote: > Hi all, > > Kafka currently use scala.util.parsing.json.JSON as its json parser and it > has a number of issues: > > * It encourages unsafe casts (returns `Option[Any]`) > * It's slow (it relies on parser combinators under the hood) > * It's not thread-safe (so external locks are needed to use it in a > concurrent environment) > * It's deprecated (it should have never been included in the standard > library in the first place) > > KAFKA-1595[1] has been filed to track this issue. > > I initially proposed a change using spray-json's AST with the jawn > parser[2]. Gwen expressed some reservations about the choice (a previous > discussion had concluded that Jackson should be used instead) and asked me > to raise the issue in the mailing list[3]. > > In order to have a fair comparison, I implemented the change using Jackson > as well[4]. I paste part of the commit message: > > "A thin wrapper over Jackson's Tree Model API is used as the replacement. > This wrapper > increases safety while providing a simple, but powerful API through the > usage of the > `DecodeJson` type class. Even though this has a maintenance cost, it makes > the API > much more convenient from Scala. A number of tests were added to verify the > behaviour of this wrapper. The Scala module for Jackson doesn't provide any > help for our current usage, so we don't > depend on it." > > A comparison between the two approaches as I see it: > > Similarities: > > 1. The code for users of the JSON library is similar > 2. No third-party dependencies > 3. Good performance > > In favour of using Jackson: > > 1. Same library for client and broker > 2. Widely used > > In favour of using spray-json and jawn: > > 1. Simple type class based API is included and it has a number of nice > features: > 1. Support for parsing into case classes (we don't use this yet, but > we could use it to make the code safer and more readable in some > cases)[5]. > 2. Very little reflection used (only for retrieving case classes > field names). > 3. Write support (could replace our `Json.encode` method). > 2. Less code to maintain (ie we don't need a wrapper to make it nice to > use from Scala) > 3. No memory overhead from wrapping the Jackson classes (probably not a > big deal) > > I am happy to go either way as both approaches have been implemented and I > am torn between the options. > > What do you think? > > Best, > Ismael > > [1] https://issues.apache.org/jira/browse/KAFKA-1595 > [2] > > https://github.com/ijuma/kafka/commit/80974afefc00eb6313a7357e7942d5d86ffce84d > [3] > > https://issues.apache.org/jira/browse/KAFKA-1595?focusedCommentId=14512881&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14512881 > [4] > > https://github.com/ijuma/kafka/commit/4ca0f1111eb37e8be2d388b60efacc19bc6788b6 > [5] The Scala module for Jackson (which is not being used in the commit > above) also supports this, but it uses a reflection-based approach instead > of type classes. >
