Thanks for all the work, Almog. For the most part, I think this KIP will be a great improvement, and IMO is almost ready to go. However, I do have a few suggestions that affect the wording more than the intent.
First, the name of the `json.decimal.serialization.format` property is pretty long, especially when it is prefixed in the Worker config or in a connector config as `key.converer.json.decimal.serialization.format` or `value.converter.json.decimal.serialization.format` . Have you considered a shorter config property name, such as maybe `decimal.format`? Is there any benefit to include "json" and "serialization" in the property name? Also, we should be clear that the value will not be case sensitive (e.g., "numeric" and "NUMERIC" would be equivalent), to keep in alignment with other enumeration literals in Connect configurations. The goal should be simple Second, the Motivation section, has the following sentence: "A new configuration for producers json.decimal.serialization.format will be introduced to the JsonConverter configuration to help control whether source converters will serialize decimals in numeric or binary formats." I agree with an earlier comment from Konstantine that "producers" here is distracting and does not mirror the normal definition of "producers" within the Kafka context. I suggest rephrasing this to something like "Introduce to the JsonConverter a new configuration property named json.decimal.serialization.format to control whether source converters will serialize decimals in numeric or binary formats." Third, the KIP should be more clear about whether the `json.decimal.serialization.format` setting does or does not affect deserialization? IIUC, the deserialization logic will always look for JSON numbers, and will always use the Schema to define whether it should convert the value to a different number type. Is that a fair statement? Fourth, the JsonSerializer and JsonDeserializer are not affected, yet are still compatible with the old and new behavior. Because the primary purpose of this new setting is to define how Connect DECIMAL logical type values are serialized in JSON documents, the JsonDeserializer will still be able to deserialize the JSON document correctly. The resulting application, however, may need to handle a wider range of numeric values. Fifth, the Compatibility section seems more complicated than perhaps it needs to be, maybe because it seems to distinguish between upgrading and setting the decimal serialization format. Maybe it would be sufficient to simply emphasize that all of the sink connectors (or consumer applications) using the JsonConverter with the `json.decimal.serialization.format=NUMERIC` setting consuming records from a set of topics be upgraded and changed *before* any of the source connectors (or other producer applications) using the JsonConverter to serialize records are changed to use the `json.decimal.serialization.format=NUMERIC` setting? It may also warrant giving more concrete advice on upgrade procedures. For example, how does a user upgrade a set of Connect workers to use this new property? Do they upgrade first and restart to ensure everything runs as-is, and then upgrade their source connectors to set `json.decimal.serialization.format=NUMERIC`via connector configurations or worker configs? Anyway, great job so far! Best regards, Randall On Thu, Aug 15, 2019 at 12:00 PM Konstantine Karantasis < konstant...@confluent.io> wrote: > Thanks! KIP reads even better for me now. > Just voted. +1 non-binding > > Konstantine > > On Wed, Aug 14, 2019 at 7:00 PM Almog Gavra <al...@confluent.io> wrote: > > > 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 > > > > > > >> > >> > > > > > > > > > > >> > >> > > > > > > > > > >> > >> > > > > > > > > >> > >> > > > > > > > >> > >> > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > >