Hey Sagar,
Thanks for the KIP! The overall proposal looks good to me, but I had some
miscellaneous notes:
1) Some general KIP-writing advice:
- You don't need to list any implementation details, only public
interfaces that are being added, modified, or
deprecated. It's better to describe your changes in words, or
occasionally psuedo-code for more complicated
changes or algorithms. The KIP is a public contract, so you generally
want to agree upon the high-level
proposal and avoid getting locked in to specific code which you may
end up wanting to change once you
start on the PR.
In this KIP, you can remove the code block under
*TimeWindowedDeserializer
*and just describe the desired
semantics: eg that you should only use the constructor within
Streams, the configs within the console consumer,
or either for a plain consumer client provided they match.
The code block under *StreamsConfig *however is a good example of
what should be in a KIP. Only one nit:
in the doc string, avoid saying "windowed key" and just say "windowed
record" or something like that.
- The *Motivation* section should be focused on any background or
additional context that's necessary to
understand the KIP, as well as the actual motivation for the change
being proposed. So here, everything under
the second bullet which begins with "The KIP also introduces
changes..." should be cut from that section and
discussed elsewhere.
- The *Proposed Changes* and *Public Interfaces* sections have a lot of
overlap and repeated content. To be
honest I personally have struggled with deciding which section to
include something in, but a good rule of thumb
is to describe the actual changes in the *Proposed Changes* section,
and then use the *Public Interfaces* section
to list any new or modified public APIs. In this case, I would move
everything to the *Proposed Changes* section
except for the code block with the deprecated config(s) and the new
config + doc string.
2) Can you make it clear that both *default.windowed.key.serde.inner* and
*default.windowed.key.serde.inner *
are being deprecated, and we're adding a new config called
*windowed.deserializer.inner.class*, not literally
renaming the existing *default.windowed.key.serde.inner* config? I think
you're hinting at this in the
*Compatibility, Deprecation, and Migration* section, but elsewhere in the
KIP it's implied that we'll be replacing
existing config which would break any applications that currently rely on
it. Please update the *Public Interfaces*
and *Proposed Changes* sections to clarify that both of these configs will
be deprecated.
3) At the end of this section you raise a question that I don't think I
quite understand, can you elaborate on this:
We can maybe enforce the removal of the deprecated configs and then enforce
> users?
>
Note: you only need to worry about deprecating these configs in the current
KIP. Once deprecated we just need to
give users enough time to migrate over to the new API, and then we can
remove them in the next major version.
The important thing for the KIP itself is just to make sure the change is
visible to users and provides a clear
migration path.
4) For the Console Consumer you say
It would be mandatory to pass windowed.deserializer.inner.class and
> window.size.ms config. <Need to check how to do this>
>
As I said above, you don't need to worry about putting how you'll implement
this in the KIP document. But to answer
your implicit question, I actually don't think you need to do anything
special for the console consumer -- the
TimeWindowedDeserializer itself will verify that both its parameters have
been set and throw an exception if not.
5) One last small suggestion: I think we should throw an exception if a
user has tried to use the new
*windowed.deserializer.inner.class *config in their Kafka Streams
application, since it's intended for use only
with/for the plain consumer client. If you agree, this is something that
should be noted in the KIP somewhere.
It may also be a good idea to note this in the config doc string as well,
so users don't try to use it as if it was a
literal replacement of the *deprecated
default.windowed.key.serde.inner* config.
What do you think?
(To clarify, I'm suggesting we check inside the KafkaStreams constructor
and throw if this config is set, but this
last bit doesn't need to go into the KIP as it's an implementation detail)
Once you've addressed the above and cleaned the KIP up a bit, I'm +1 -- the
changes you propose make sense to me.
Cheers,
Sophie
On Mon, Mar 22, 2021 at 3:28 AM Sagar <sagarmeansoc...@gmail.com> wrote:
> Hi All,
>
> I would like to start a discussion on the following KIP:
>
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=177047930
>
> Thanks!
> Sagar.
>
