[DISCUSS] Deprecate KeyValue schema factory methods with Class parameters

[Yunze Xu <yz...@streamnative.io.INVALID>]

Hi folks,

Recently I'm looking into the KeyValue schema and found **FOUR**
static methods in `Schema` to create a `KeyValueSchema<K, V>` object:

1. KeyValue(Class<K> key, Class<V> value);
2. KeyValue(Class<K> key, Class<V> value, SchemaType type);
3. KeyValue(Schema<K> key, Schema<V> value);
4. KeyValue(Schema<K> key, Schema<V> value, KeyValueEncodingType keyValueEncodingType);

IMO, having too many overloads is not user-friendly. I turned to the
official website and found overload 4 is used. The overload 3 is just
a simple version of overload 4 whose encoding type is `INLINE`, but
it's not clear. I opened a PR
https://github.com/apache/pulsar/pull/17256 to make it more clear.

However, for overload 1 and 2, I can only find two references in unit
tests `testAllowNullAvroSchemaCreate` and
`testAllowNullJsonSchemaCreate`. From the very simple JavaDocs, it
looks like they are only available for JSON and AVRO schemas.

Then I found the original PR to introduce these overloads:
https://github.com/apache/pulsar/pull/2885

Code has been changed a lot since that. It looks other codes are not
much related to these two overloads now. IMO, we should not encourage
users to use these two overloads, so I suggest marking them as
deprecated and might remove them in future releases.


Thanks,
Yunze

Re: [DISCUSS] Deprecate KeyValue schema factory methods with Class parameters

[Qiang Huang <qi...@gmail.com>]

Nice catch. LGTM.

Yunze Xu <yz...@streamnative.io.invalid> 于2022年8月24日周三 16:29写道：

> Hi folks,
>
> Recently I'm looking into the KeyValue schema and found **FOUR**
> static methods in `Schema` to create a `KeyValueSchema<K, V>` object:
>
> 1. KeyValue(Class<K> key, Class<V> value);
> 2. KeyValue(Class<K> key, Class<V> value, SchemaType type);
> 3. KeyValue(Schema<K> key, Schema<V> value);
> 4. KeyValue(Schema<K> key, Schema<V> value, KeyValueEncodingType
> keyValueEncodingType);
>
> IMO, having too many overloads is not user-friendly. I turned to the
> official website and found overload 4 is used. The overload 3 is just
> a simple version of overload 4 whose encoding type is `INLINE`, but
> it's not clear. I opened a PR
> https://github.com/apache/pulsar/pull/17256 to make it more clear.
>
> However, for overload 1 and 2, I can only find two references in unit
> tests `testAllowNullAvroSchemaCreate` and
> `testAllowNullJsonSchemaCreate`. From the very simple JavaDocs, it
> looks like they are only available for JSON and AVRO schemas.
>
> Then I found the original PR to introduce these overloads:
> https://github.com/apache/pulsar/pull/2885
>
> Code has been changed a lot since that. It looks other codes are not
> much related to these two overloads now. IMO, we should not encourage
> users to use these two overloads, so I suggest marking them as
> deprecated and might remove them in future releases.
>
>
> Thanks,
> Yunze
>
>
>
>
>

-- 
BR,
Qiang Huang