Thanks for the KIP!
LGTM, as well!
Best,
Bruno
On 02.12.20 00:41, Walker Carlson wrote:
Thanks for making these changes. It makes more sense now to me. Overall LGTM
walker
On Tue, Dec 1, 2020 at 3:39 PM Sophie Blee-Goldman <sop...@confluent.io>
wrote:
Thanks for the KIP! I'm happy with the state of things after your latest
update,
LGTM
Sophie
On Tue, Dec 1, 2020 at 2:26 PM Leah Thomas <ltho...@confluent.io> wrote:
Hi Matthias,
Yeah I think it should, good catch. That should also answer Walker's
question about why we have an option for `withLoggingEnabled()` even
though
that's the default. Passing in a new map of configs could allow the user
to
configure the log differently than the default. I've updated the KIP to
reflected the added parameter and an added variable, `topicConfig` to
store
the map of configs.
Best,
Leah
On Mon, Nov 30, 2020 at 5:35 PM Matthias J. Sax <mj...@apache.org>
wrote:
Thanks for the KIP Leah.
Should `withLoggingEnabled()` take a `Map<String, String> config`
similar to the one from `Materialized`?
-Matthias
On 11/30/20 12:22 PM, Walker Carlson wrote:
Ah. That makes sense. Thank you for fixing that.
One minor question. If the default is enabled is there any case
where a
user would turn logging off then back on again? I can see having the
enableLoging for completeness so it's not that important probably.
Anyways other than that it looks good!
Walker
On Mon, Nov 30, 2020 at 12:06 PM Leah Thomas <ltho...@confluent.io>
wrote:
Hey Walker,
Thanks for your response.
1. Ah yeah thanks for the catch, that was held over from copy/paste.
Both
functions should take no parameters, as they just `loggingEnabled`
to
true
or false. I've removed the `WindowBytesStoreSupplier
otherStoreSupplier`
from the methods in the KIP
2. I think the fix to 1 answers this question, otherwise, I'm not
quite
sure what you're asking. With the updated method calls, there
shouldn't
be
any duplication.
Cheers,
Leah
On Mon, Nov 30, 2020 at 12:14 PM Walker Carlson <
wcarl...@confluent.io>
wrote:
Hello Leah,
Thank you for the KIP.
I had a couple questions that maybe you can expand on from what is
on
the
KIP.
1) Why are we enabling/disabling the logging by passing in a
`WindowBytesStoreSupplier`?
It seems to me that these two things should be separate.
2) There is already `withThisStoreSupplier(final
WindowBytesStoreSupplier
otherStoreSupplier)` and `withOtherStoreSupplier(final
WindowBytesStoreSupplier otherStoreSupplier)`. Why do we need to
duplicate
them when the `retentionPeriod` can be set through them?
Thanks,
Walker
On Mon, Nov 30, 2020 at 8:53 AM Leah Thomas <ltho...@confluent.io>
wrote:
After reading through
https://issues.apache.org/jira/browse/KAFKA-9921
I
removed the option to enable/disable caching for `StreamJoined`,
as
caching
will always be disabled because we retain duplicates.
I updated the KIP accordingly, it now adds only `enableLogging`
as a
config.
On Mon, Nov 30, 2020 at 9:54 AM Leah Thomas <ltho...@confluent.io
wrote:
Hi all,
I'd like to kick-off the discussion for KIP-689: Extend
`StreamJoined`
to
allow more store configs. This builds off the work of KIP-479
<
https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join
to
add options to enable/disable both logging and caching for stream
join
stores.
KIP is here:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-689%3A+Extend+%60StreamJoined%60+to+allow+more+store+configs
Looking forward to hearing your thoughts,
Leah
