[jira] [Comment Edited] (KAFKA-12313) Consider deprecating the default.windowed.serde.inner.class configs

["Sagar Rao (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/KAFKA-12313?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17301200#comment-17301200 ] 

Sagar Rao edited comment on KAFKA-12313 at 3/15/21, 3:11 AM:
-------------------------------------------------------------

I took a pass through the codebase and the KIP discussion and here are my thoughts:

1) Renaming "default.windowed.key.serde.inner" to "windowed.key.serde.inner" makes sense. Having default in the name gives the sense that it is something that will be used when the use doesn't supply it, but instead that doesn't seem to be the case.

2) Removing  "default.windowed.value.serde.inner" also makes sense to me as you explained that there is no concept of a Windowed Value.

3) Regarding windowSize parameter, what i understood from the code is that null values won't get set in windowSize because of this block of code in configure in TimeWindowedDeserializer:

 
{code:java}
// code placeholder
final Long configWindowSize;
if (configs.get(StreamsConfig.WINDOW_SIZE_MS_CONFIG) instanceof String) {
    configWindowSize = Long.parseLong((String) configs.get(StreamsConfig.WINDOW_SIZE_MS_CONFIG));
} else {
    configWindowSize = (Long) configs.get(StreamsConfig.WINDOW_SIZE_MS_CONFIG);
}
if (windowSize != null && configWindowSize != null) {
    throw new IllegalArgumentException("Window size should not be set in both the time windowed deserializer constructor and the window.size.ms config");
} else if (windowSize == null && configWindowSize == null) {
    throw new IllegalArgumentException("Window size needs to be set either through the time windowed deserializer " +
        "constructor or the window.size.ms config but not both");
} else {
    windowSize = windowSize == null ? configWindowSize : windowSize;
}

{code}
The option to set windowSize to Long.MAX_VALUE is deprecated, so unless somebody actually sets it deliberately, I think it should not get set. If needed, we can add some warning etc here to make the user aware of the consequences of setting it to Long.MAX_VALUE?

 

4) Regaring windowed.key.serde.inner, if Deserialiser is initialised through default constructor, then within the configure method it does get to the value defined by the user. If even that is null, then  Utils.newInstance throws a ClassNotFoundException. So, effectively, it is ensuring that the user needs to have passed the Deserialiser through either the code or via the StreamConfig.

5) Lastly, for console consumer, would it make sense to ensure that both windowSerde parameters are passed for this case? i haven't looked at the console consumer code, but would it be possible to set these conditions when somebody wants to run console consumer for windowed stores?

 

Plz let me know if these points make sense.

 


was (Author: sagarrao):
I took a pass through the codebase and the KIP discussion and here are my thoughts:

1) Renaming "default.windowed.key.serde.inner" to "windowed.key.serde.inner" makes sense. Having default in the name gives the sense that it is something that will be used when the use doesn't supply it, but instead that doesn't seem to be the case.

2) Removing  "default.windowed.value.serde.inner" also makes sense to me as you explained that there is no concept of a Windowed Value.

3) Regarding windowSize parameter, what i understood from the code is that null values won't get set in windowSize because of this block of code in configure in TimeWindowedDeserializer:

 
{code:java}
// code placeholder
final Long configWindowSize;
if (configs.get(StreamsConfig.WINDOW_SIZE_MS_CONFIG) instanceof String) {
    configWindowSize = Long.parseLong((String) configs.get(StreamsConfig.WINDOW_SIZE_MS_CONFIG));
} else {
    configWindowSize = (Long) configs.get(StreamsConfig.WINDOW_SIZE_MS_CONFIG);
}
if (windowSize != null && configWindowSize != null) {
    throw new IllegalArgumentException("Window size should not be set in both the time windowed deserializer constructor and the window.size.ms config");
} else if (windowSize == null && configWindowSize == null) {
    throw new IllegalArgumentException("Window size needs to be set either through the time windowed deserializer " +
        "constructor or the window.size.ms config but not both");
} else {
    windowSize = windowSize == null ? configWindowSize : windowSize;
}

{code}
The option to set windowSize to Long.MAX_VALUE is deprecated, so unless somebody actually sets it deliberately, I think it should not get set. If needed, we can add some warning etc here to make the user aware of the consequences of setting it to Long.MAX_VALUE?

 

4) Regaring windowed.key.serde.inner, if Deserialiser is initialised through default constructor, then within the configure method it does get to the value defined by the user. If even that is null, then  Utils.newInstance throws a ClassNotFoundException. So, effectively, it is ensuring that the user needs to have passed the Deserialiser through either the code or via the StreamConfig.

5) Lastly, for console consumer, would it make sense to ensure that both windowSerde parameters are passed for this case? i think it should be easier via the console consumer params.

 

Plz let me know if these points make sense.

 

> Consider deprecating the default.windowed.serde.inner.class configs
> -------------------------------------------------------------------
>
>                 Key: KAFKA-12313
>                 URL: https://issues.apache.org/jira/browse/KAFKA-12313
>             Project: Kafka
>          Issue Type: Improvement
>          Components: streams
>            Reporter: A. Sophie Blee-Goldman
>            Assignee: Sagar Rao
>            Priority: Major
>              Labels: needs-kip
>             Fix For: 3.0.0
>
>
> During the discussion of KIP-659 we discussed whether it made sense to have a "default" class for the serdes of windowed inner classes across Streams. Using these configs instead of specifying an actual Serde object can lead to subtle bugs, since the WindowedDeserializer requires a windowSize in addition to the inner class. If the default constructor is invoked, as it will be when falling back on the config, this windowSize defaults to MAX_VALUE. 
> If the downstream program doesn't care about the window end time in the output, then this can go unnoticed and technically there is no problem. But if anything does depend on the end time, or the user just wants to manually read the output for testing purposes, then the MAX_VALUE will result in a garbage timestamp.
> We should consider whether the convenience of specifying a config instead of instantiating a Serde in each operator is really worth the risk of a user accidentally failing to specify a windowSize



--
This message was sent by Atlassian Jira
(v8.3.4#803005)