[ 
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)

Reply via email to