Thanks for the review Konstantine!
I think the terminology suggestion definitely makes things clearer - I will
update the documentation based on your suggestion (e.g. s/Consumer/Sink
Converter/g and s/Producer/Source Converter/g).
Cheers,
Almog
On Wed, Aug 14, 2019 at 8:13 AM Konstantine Karantasis <
konstant...@confluent.io> wrote:
> Thanks Almog for preparing this KIP!
> I think it will improve usability and troubleshooting with JSON data a lot.
>
> The finalized plan seems quite concrete now. I also liked that some
> implementation specific implications (such as setting the ObjectMapper to
> deserialize floating point as BigDecimal) are highlighted in the KIP.
>
> Still, as I was reading the KIP, the main obstacle I encountered was around
> terminology. I couldn't get used to reading "producer" and "consumer" and
> not thinking in terms of Kafka producers and consumers - which are not
> relevant to what this KIP proposes. Thus, I'd suggest replacing
> "Producer(s)" with "Source Converter(s)" and "Consumer(s)" with "Sink
> Converter(s)" (even if "Converter used by Source Connector" or "Converter
> used by Sink Connector" would be even more accurate - maybe this could be
> an explanation in a footnote). Terminology around converters has been
> tricky in the past and adding producers/consumers in the mix might add to
> the confusion.
>
> Another example where I'd apply this different terminology would be to a
> phrase such as the following:
> "Because of this, users must take care to first ensure that all consumers
> have upgraded to the new code before upgrading producers to make use of the
> NUMERIC serialization format."
> which I'd write
> "Because of this, users must take care to first ensure that all sink
> connectors have upgraded to the new converter code before upgrading source
> connectors to make use of the NUMERIC serialization format in
> JsonConverter."
>
> Let me know if you think this suggestion makes the KIP easier to follow.
> Otherwise I think it's a solid proposal.
>
> I'm concluding with a couple of nits:
>
> - "Upgraded Producer with BASE64 serialization, Legacy Consumer: this
> scenario is okay as the upgraded ~producer~ consumer will be able to read
> binary as today" (again according to my suggestion above, it could be as
> the upgraded source converter ...)
>
> - "consumers cannot consumer NUMERIC data. " -> "consumers cannot read
> NUMERIC data"
>
> Best,
> Konstantine
>
> On Fri, Aug 9, 2019 at 6:37 PM Almog Gavra <al...@confluent.io> wrote:
>
> > Good catches! Fixed :)
> >
> > On Thu, Aug 8, 2019 at 10:36 PM Arjun Satish <arjun.sat...@gmail.com>
> > wrote:
> >
> > > Cool!
> > >
> > > Couple of nits:
> > >
> > > - In public interfaces, typo: *json.decimal.serialization.fromat*
> > > - In public interfaces, you use the term "HEX" instead of "BASE64".
> > >
> > >
> > >
> > > On Wed, Aug 7, 2019 at 9:51 AM Almog Gavra <al...@confluent.io> wrote:
> > >
> > > > EDIT: everywhere I've been using "HEX" I meant to be using "BASE64".
> I
> > > will
> > > > update the KIP to reflect this.
> > > >
> > > > On Wed, Aug 7, 2019 at 9:44 AM Almog Gavra <al...@confluent.io>
> wrote:
> > > >
> > > > > Thanks for the feedback Arjun! I'm happy changing the default
> config
> > to
> > > > > HEX instead of BINARY, no strong feelings there.
> > > > >
> > > > > I'll also clarify the example in the KIP to be clearer:
> > > > >
> > > > > - serialize the decimal field "foo" with value "10.2345" with the
> HEX
> > > > > setting: {"foo": "D3J5"}
> > > > > - serialize the decimal field "foo" with value "10.2345" with the
> > > NUMERIC
> > > > > setting: {"foo": 10.2345}
> > > > >
> > > > > With regards to the precision issue, that was my original concern
> as
> > > well
> > > > > (and why I originally suggested a TEXT format). Many JSON
> > deserializers
> > > > > (e.g. Jackson with
> > DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS),
> > > > > however, have the ability to deserialize decimals correctly so I
> will
> > > > > configure that as the default for Connect's JsonDeserializer. It's
> > > > probably
> > > > > a good idea to call out that using other deserializers must be done
> > > with
> > > > > care - I will add that documentation to the serialization config.
> > > > >
> > > > > Note that there would not be an issue on the _serialization_ side
> of
> > > > > things as Jackson respects BigDecimal.
> > > > >
> > > > > Almog
> > > > >
> > > > > On Tue, Aug 6, 2019 at 11:23 PM Arjun Satish <
> arjun.sat...@gmail.com
> > >
> > > > > wrote:
> > > > >
> > > > >> hey Almog, nice work! couple of thoughts (hope I'm not late since
> > you
> > > > >> started the voting thread already):
> > > > >>
> > > > >> can you please add some examples to show the changes that you are
> > > > >> proposing. makes me think that for a given decimal number, we will
> > > have
> > > > >> two
> > > > >> encodings: “asHex” and “asNumber”.
> > > > >>
> > > > >> should we call the default config value “HEX” instead of “BINARY”?
> > > > >>
> > > > >> Should we call out the fact that JS systems might be susceptible
> to
> > > > double
> > > > >> precision round offs with the new numeric format? here are some
> > people
> > > > >> discussing a similar problem
> > > > >> https://github.com/EventStore/EventStore/issues/1541
> > > > >>
> > > > >> On Tue, Aug 6, 2019 at 1:40 PM Almog Gavra <al...@confluent.io>
> > > wrote:
> > > > >>
> > > > >> > Hello Everyone,
> > > > >> >
> > > > >> > Summarizing an in-person discussion with Randall (this is copied
> > > from
> > > > >> the
> > > > >> > KIP):
> > > > >> >
> > > > >> > The original KIP suggested supporting an additional
> > representation -
> > > > >> base10
> > > > >> > encoded text (e.g. `{"asText":"10.2345"}`). This causes issues
> > > because
> > > > >> it
> > > > >> > is impossible to disambiguate between TEXT and BINARY without an
> > > > >> additional
> > > > >> > config - furthermore, this makes the migration from one to the
> > other
> > > > >> nearly
> > > > >> > impossible because it would require that all consumers stop
> > > consuming
> > > > >> and
> > > > >> > producers stop producing and atomically updating the config on
> all
> > > of
> > > > >> them
> > > > >> > after deploying the new code, or waiting for the full retention
> > > period
> > > > >> to
> > > > >> > pass - neither option is viable. The suggestion in the KIP is
> > > strictly
> > > > >> an
> > > > >> > improvement over the existing behavior, even if it doesn't
> support
> > > all
> > > > >> > combinations.
> > > > >> >
> > > > >> > It seems that since most real-world use cases actually use the
> > > numeric
> > > > >> > representation (not string) we can consider this an improvement.
> > > With
> > > > >> the
> > > > >> > new suggestion, we don't need a deserialization configuration
> > (only
> > > a
> > > > >> > serialization option) and the consumers will be able to always
> > > > >> > automatically determine the serialization format.
> > > > >> >
> > > > >> > Based on this, I'll be opening up the simplified version of the
> > KIP
> > > > to a
> > > > >> > vote.
> > > > >> >
> > > > >> > Almog
> > > > >> >
> > > > >> > On Mon, Jul 29, 2019 at 9:29 AM Almog Gavra <al...@confluent.io
> >
> > > > wrote:
> > > > >> >
> > > > >> > > I'm mostly happy with your current suggestion (two configs,
> one
> > > for
> > > > >> > > serialization and one for deserialization) and your
> > implementation
> > > > >> > > suggestion. One thing to note:
> > > > >> > >
> > > > >> > > > We should _always_ be able to deserialize a standard JSON
> > > > >> > > > number as a decimal
> > > > >> > >
> > > > >> > > I was doing some research into decimals and JSON, and I can
> > > imagine
> > > > a
> > > > >> > > compelling reason to require string representations to avoid
> > > losing
> > > > >> > > precision and to be certain that whomever is sending the data
> > > isn't
> > > > >> > losing
> > > > >> > > precision (e.g. https://stackoverflow.com/a/38357877/2258040
> ).
> > > > >> > >
> > > > >> > > I'm okay with always allowing numerics, but thought it's worth
> > > > raising
> > > > >> > the
> > > > >> > > thought.
> > > > >> > >
> > > > >> > > On Mon, Jul 29, 2019 at 4:57 AM Andy Coates <
> a...@confluent.io>
> > > > >> wrote:
> > > > >> > >
> > > > >> > >> The way I see it, we need to control two seperate things:
> > > > >> > >>
> > > > >> > >> 1. How do we _deserialize_ a decimal type if we encounter a
> > text
> > > > >> node in
> > > > >> > >> the JSON? (We should _always_ be able to deserialize a
> > > standard
> > > > >> JSON
> > > > >> > >> number as a decimal).
> > > > >> > >> 2. How do we chose how we want decimals to be _serialized_.
> > > > >> > >>
> > > > >> > >> This looks to fits well with your second suggestion of
> slightly
> > > > >> > different
> > > > >> > >> configs names for serialization vs deserialization.
> > > > >> > >> a, For deserialization we only care about how to handle text
> > > > nodes: `
> > > > >> > >> deserialization.decimal.*text*.format`, which should only
> have
> > > two
> > > > >> valid
> > > > >> > >> values BINARY | TEXT.
> > > > >> > >> b. For serialization we need all three:
> > > > >> `serialization.decimal.format`,
> > > > >> > >> which should support all three options: BINARY | TEXT |
> > NUMERIC.
> > > > >> > >>
> > > > >> > >> Implementation wise, I think these should be two separate
> > enums,
> > > > >> rather
> > > > >> > >> than one shared enum and throwing an error if the
> deserializer
> > is
> > > > >> set to
> > > > >> > >> NUMERIC. Mainly as this means the enums reflect the options
> > > > >> available,
> > > > >> > >> rather than this being hidden in config checking code. But
> > > that's
> > > > a
> > > > >> > minor
> > > > >> > >> implementation detail.
> > > > >> > >>
> > > > >> > >> Personally, I'd be tempted to have the BINARY value named
> > > something
> > > > >> like
> > > > >> > >> `LEGACY` or `LEGACY_BINARY` as a way of encouraging users to
> > move
> > > > >> away
> > > > >> > >> from
> > > > >> > >> it.
> > > > >> > >>
> > > > >> > >> It's a real shame that both of these settings require a
> default
> > > of
> > > > >> > BINARY
> > > > >> > >> for backwards compatibility, but I agree that discussions /
> > plans
> > > > >> around
> > > > >> > >> switching the defaults should not block this KIP.
> > > > >> > >>
> > > > >> > >> Andy
> > > > >> > >>
> > > > >> > >>
> > > > >> > >> On Thu, 25 Jul 2019 at 18:26, Almog Gavra <
> al...@confluent.io>
> > > > >> wrote:
> > > > >> > >>
> > > > >> > >> > Thanks for the replies Andy and Andrew (2x Andy?)!
> > > > >> > >> >
> > > > >> > >> > > Is the text decimal a base16 encoded number, or is it
> > base16
> > > > >> encoded
> > > > >> > >> > binary
> > > > >> > >> > > form of the number?
> > > > >> > >> >
> > > > >> > >> > The conversion happens as
> > decimal.unscaledValue().toByteArray()
> > > > and
> > > > >> > then
> > > > >> > >> > the byte array is converted to a hex string, so it's
> > definitely
> > > > the
> > > > >> > >> binary
> > > > >> > >> > form of the number converted to base16. Whether or not
> that's
> > > the
> > > > >> same
> > > > >> > >> as
> > > > >> > >> > the base16 encoded number is a good question (toByteArray
> > > > returns a
> > > > >> > byte
> > > > >> > >> > array containing a signed, big-endian, two's complement
> > > > >> representation
> > > > >> > >> of
> > > > >> > >> > the big integer).
> > > > >> > >> >
> > > > >> > >> > > One suggestion I have is to change the proposed new
> config
> > to
> > > > >> only
> > > > >> > >> affect
> > > > >> > >> > > decimals stored as text, i.e. to switch between the
> current
> > > > >> base16
> > > > >> > and
> > > > >> > >> > the
> > > > >> > >> > > more common base10. Then add another config to the
> > > serializer
> > > > >> only
> > > > >> > >> that
> > > > >> > >> > > controls if decimals should be serialized as text or
> > numeric.
> > > > >> > >> >
> > > > >> > >> > I think we need to be able to handle all mappings from
> > > > >> serialization
> > > > >> > >> format
> > > > >> > >> > to deserialization format (e.g. read in BINARY and output
> > > TEXT),
> > > > >> > which I
> > > > >> > >> > think would be impossible with the alternative suggestion.
> I
> > > > agree
> > > > >> > that
> > > > >> > >> > automatically deserializing numerics is valuable. I see two
> > > other
> > > > >> ways
> > > > >> > >> to
> > > > >> > >> > get this, both keeping the serialization.format config the
> > > same:
> > > > >> > >> >
> > > > >> > >> > - have json.decimal.deserialization.format accept all three
> > > > >> formats.
> > > > >> > if
> > > > >> > >> set
> > > > >> > >> > to BINARY/TEXT, numerics would be automatically supported.
> If
> > > set
> > > > >> to
> > > > >> > >> > NUMERIC, then any string coming in would result in
> > > > deserialization
> > > > >> > error
> > > > >> > >> > (defaults to BINARY for backwards compatibility)
> > > > >> > >> > - change json.decimal.deserialization.format to
> > > > >> > >> > json.decimal.deserialization.string.format which accepts
> only
> > > > >> > >> BINARY/TEXT
> > > > >> > >> > (defaults to BINARY for backwards compatibility)
> > > > >> > >> >
> > > > >> > >> > > would be a breaking change in that things that previously
> > > > failed
> > > > >> > would
> > > > >> > >> > > suddenly start deserializing. This is a price I'm
> willing
> > to
> > > > >> pay.
> > > > >> > >> >
> > > > >> > >> > I agree. I'm willing to pay this price too.
> > > > >> > >> >
> > > > >> > >> > > IMHO, we should then plan to switch the default of
> decimal
> > > > >> > >> serialization
> > > > >> > >> > to
> > > > >> > >> > > numeric, and text serialization to base 10 in the next
> > major
> > > > >> > release.
> > > > >> > >> >
> > > > >> > >> > I think that can be a separate discussion, I don't want to
> > > block
> > > > >> this
> > > > >> > >> KIP
> > > > >> > >> > on it.
> > > > >> > >> >
> > > > >> > >> > Thoughts?
> > > > >> > >> >
> > > > >> > >> > On Thu, Jul 25, 2019 at 6:35 AM Andrew Otto <
> > > o...@wikimedia.org>
> > > > >> > wrote:
> > > > >> > >> >
> > > > >> > >> > > This is a bit orthogonal, but in JsonSchemaConverter I
> use
> > > > >> > >> JSONSchemas to
> > > > >> > >> > > indicate whether a JSON number should be deserialized as
> an
> > > > >> integer
> > > > >> > >> or a
> > > > >> > >> > > decimal
> > > > >> > >> > > <
> > > > >> > >> > >
> > > > >> > >> >
> > > > >> > >>
> > > > >> >
> > > > >>
> > > >
> > >
> >
> https://github.com/ottomata/kafka-connect-jsonschema/blob/master/src/main/java/org/wikimedia/kafka/connect/jsonschema/JsonSchemaConverter.java#L251-L261
> > > > >> > >> > > >.
> > > > >> > >> > > Not everyone is going to have JSONSchemas available when
> > > > >> converting,
> > > > >> > >> but
> > > > >> > >> > if
> > > > >> > >> > > you do, it is an easy way to support JSON numbers as
> > > decimals.
> > > > >> > >> > >
> > > > >> > >> > > Carry on! :)
> > > > >> > >> > >
> > > > >> > >> > > On Thu, Jul 25, 2019 at 9:12 AM Andy Coates <
> > > a...@confluent.io
> > > > >
> > > > >> > >> wrote:
> > > > >> > >> > >
> > > > >> > >> > > > Hi Almog,
> > > > >> > >> > > >
> > > > >> > >> > > > Like the KIP - I think being able to support decimals
> in
> > > JSON
> > > > >> in
> > > > >> > the
> > > > >> > >> > same
> > > > >> > >> > > > way most other systems do is a great improvement.
> > > > >> > >> > > >
> > > > >> > >> > > > It's not 100% clear to me from the KIP what the current
> > > > format
> > > > >> is.
> > > > >> > >> Is
> > > > >> > >> > > the
> > > > >> > >> > > > text decimal a base16 encoded number, or is it base16
> > > encoded
> > > > >> > binary
> > > > >> > >> > form
> > > > >> > >> > > > of the number? (I've not tried to get my head around if
> > > these
> > > > >> two
> > > > >> > >> are
> > > > >> > >> > > even
> > > > >> > >> > > > different!)
> > > > >> > >> > > >
> > > > >> > >> > > > One suggestion I have is to change the proposed new
> > config
> > > to
> > > > >> only
> > > > >> > >> > affect
> > > > >> > >> > > > decimals stored as text, i.e. to switch between the
> > current
> > > > >> base16
> > > > >> > >> and
> > > > >> > >> > > the
> > > > >> > >> > > > more common base10. Then add another config to the
> > > > serialzier
> > > > >> > only
> > > > >> > >> > that
> > > > >> > >> > > > controls if decimals should be serialized as text or
> > > numeric.
> > > > >> The
> > > > >> > >> > > benefit
> > > > >> > >> > > > of this approach is it allows us to enhance the
> > > deserializer
> > > > to
> > > > >> > >> > > > automatically handle numeric decimals even without any
> > > config
> > > > >> > >> having to
> > > > >> > >> > > be
> > > > >> > >> > > > set, i.e. default config in the deserializer would be
> > able
> > > to
> > > > >> > handle
> > > > >> > >> > > > numeric decimals. Of course, this is a two edged
> sword:
> > > this
> > > > >> > would
> > > > >> > >> > make
> > > > >> > >> > > > the deserializer work out of the box with numeric
> > decimals,
> > > > >> > (yay!),
> > > > >> > >> but
> > > > >> > >> > > > would be a breaking change in that things that
> previously
> > > > >> failed
> > > > >> > >> would
> > > > >> > >> > > > suddenly start deserializing. This is a price I'm
> > willing
> > > to
> > > > >> pay.
> > > > >> > >> > > >
> > > > >> > >> > > > IMHO, we should then plan to switch the default of
> > decimal
> > > > >> > >> > serialization
> > > > >> > >> > > to
> > > > >> > >> > > > numeric, and text serialization to base 10 in the next
> > > major
> > > > >> > >> release.
> > > > >> > >> > > > (With upgrade notes to match). Though I know this is
> more
> > > > >> > >> contentious,
> > > > >> > >> > I
> > > > >> > >> > > > think it moves us forward in a much more standard way
> > that
> > > > the
> > > > >> > >> current
> > > > >> > >> > > > encoding of decimals.
> > > > >> > >> > > >
> > > > >> > >> > > > On Tue, 25 Jun 2019 at 01:03, Almog Gavra <
> > > > al...@confluent.io>
> > > > >> > >> wrote:
> > > > >> > >> > > >
> > > > >> > >> > > > > Hi Everyone!
> > > > >> > >> > > > >
> > > > >> > >> > > > > Kicking off discussion for a new KIP:
> > > > >> > >> > > > >
> > > > >> > >> > > > >
> > > > >> > >> > > >
> > > > >> > >> > >
> > > > >> > >> >
> > > > >> > >>
> > > > >> >
> > > > >>
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-481%3A+SerDe+Improvements+for+Connect+Decimal+type+in+JSON
> > > > >> > >> > > > >
> > > > >> > >> > > > > For those who are interested, I have a prototype
> > > > >> implementation
> > > > >> > >> that
> > > > >> > >> > > > helped
> > > > >> > >> > > > > guide my design:
> > https://github.com/agavra/kafka/pull/1
> > > > >> > >> > > > >
> > > > >> > >> > > > > Cheers,
> > > > >> > >> > > > > Almog
> > > > >> > >> > > > >
> > > > >> > >> > > >
> > > > >> > >> > >
> > > > >> > >> >
> > > > >> > >>
> > > > >> > >
> > > > >> >
> > > > >>
> > > > >
> > > >
> > >
> >
>
