After reading John's latest response, I agree as well.
+1(binding)
-Bill
On Tue, Feb 26, 2019 at 9:09 PM Matthias J. Sax
wrote:
> Florian,
>
> Thanks for updating the KIP.
>
> I missed the missing `static` on `Suppressed#withName()` and I agree
> with John and Guozhang.
>
> Don't have any
Florian,
Thanks for updating the KIP.
I missed the missing `static` on `Suppressed#withName()` and I agree
with John and Guozhang.
Don't have any further comments. And thanks for splitting the PR!
@Guozhang: there is already a VOTE thread.
-Matthias
On 2/26/19 3:26 PM, Guozhang Wang
Bill, John, thanks for your comments.
I agree with John that we can leave Suppressed to not have a static method
`as` for now since it is not useful at the moment. Assuming that is agreed
on, I think we can move on to the voting process.
Guozhang
On Tue, Feb 26, 2019 at 2:56 PM John Roesler
Thanks for the feedback, Bill.
It might be a good idea to hold of on adding the static method to
Suppressed for now...
Unlike the other operator/config-class pairs, `suppress` has no "default"
mode. That is, there is no `KTable.suppress()` method with no arguments.
So, when using it, you must
Hi Florian,
Thanks for the update to the KIP.
As for changing the name for "Suppressed#withName", I believe we can update
the table in KIP to say "Suppressed#as" as the KIP states that:
>> In addition, we propose to add a new static method with the following
signature to each of those class
Hi Matthias, Bill,
I've updated the KIP with your last feedbacks. However, you have suggested
to rename : `Suppressed#withName(String)`
withName is not a static method like Joined.named was. withName method is
part of the new interface NameOperation.
In addition, I've split the PR in 5 commits
Hi Florian,
Overall the KIP LGTM. Once you've addressed the final comments from
Matthias I think we can put this up for a vote.
Thanks,
Bill
On Wed, Feb 20, 2019 at 9:42 PM Matthias J. Sax
wrote:
> Florian,
>
> thanks for updating the KIP (and no worries for late reply -- 2.2
> release kept
Florian,
thanks for updating the KIP (and no worries for late reply -- 2.2
release kept us busy anyway...). Overall LGTM.
Just some nits:
KStream-Table:
Do we need to list the existing stream-globalTable join methods in the
first table (thought it should only contain new/changing methods).
Hi Matthias,
Regaridng your feedback, I've updated the KIP and PR in a way that state
store are only named regarding the provided Materialized.
I have also overload the methods: join(GlobalKTable, KeyValueMapper,
ValueJoiner)` and `leftJoin(GlobalKTable, KeyValueMapper, ValueJoiner)`
(Sorry my
I was reading the KIP again, and there are still some open question and
inconsistencies:
For example for `KGroupedStream#count(Named)` the KIP says, that only
the processor will be named, while the state store name will be `PREFIX
+ COUNT` (ie, an auto-generated name). Additionally, for
+1 for me on Guozhang's proposal for changes to Joined.
Thanks,
Bill
On Thu, Jan 17, 2019 at 5:55 PM Matthias J. Sax
wrote:
> Thanks for all the follow up comments!
>
> As I mentioned earlier, I am ok with adding overloads instead of using
> Materialized to specify the processor name. Seems
Thanks for all the follow up comments!
As I mentioned earlier, I am ok with adding overloads instead of using
Materialized to specify the processor name. Seems this is what the
majority of people prefers.
I am also +1 on Guozhang's suggestion to deprecate `static
Joined#named()` and replace it
Wow that's a lot of discussions in 6 days! :) Just catching up and sharing
my two cents here:
1. Materialized: I'm inclined to not let Materialized extending Named and
add the overload as well. All the rationales have been very well summarized
before. Just to emphasize on John's points:
Sounds good to me.
Thanks,
Bill
On Thu, Jan 17, 2019 at 4:43 PM Florian Hussonnois
wrote:
> Sorry, I've sent my previous mail to quickly. Unlike the Consumed, Produced
> and Grouped classes, the Joined class does have getter methods. So I
> propose to keep the name() method only for this
Sorry, I've sent my previous mail to quickly. Unlike the Consumed, Produced
and Grouped classes, the Joined class does have getter methods. So I
propose to keep the name() method only for this class.
For other classes the name will be accessible through XXXInternal classes.
Le jeu. 17 janv. 2019
Just to chime in regarding NamedInternal. That was my bad mental model to
blame. It is indeed coercion, not casting. Even more relevant, I'm not a
fan of the XInternal pattern, but it is the pattern we have. It would be
worse to start carving out exceptions.
So I agree that we should have:
*
Thanks for your feedback. So I will remove the name() method from the
NamedOperation interface.
After a first look, I will need to introduce a new class JoinedInternal
Le jeu. 17 janv. 2019 à 19:09, Bill Bejeck a écrit :
> I'm getting caught up with the current state of this KIP.
>
> I agree
I'm getting caught up with the current state of this KIP.
I agree that the question on what to do with overloads is a difficult one
to answer.
Both John and Matthias have laid out their thoughts thoroughly, and the
points made by both resonate with me.
I've spent some time thinking about this,
Thanks for the details John.
While I understand your argument that it is no optimal to use
`Materialized` to set the processor name, I still slightly prefer this
option, because adding more overloads seems to be even worse to me.
But I would also not block this KIP if the majority of people
Hi Matthias,
One thing that we discussed earlier was to avoid creating ambiguity by
conflating config objects that configure an operation (like Grouped) with
config objects that configure an aspect of the operation (like
Materialized).
It is natural for the Grouped config to extend Named, as
While I understand that it should be possible to specify store name and
processor name independent from each other, it's still unclear to me,
why we cannot use the `Materialized` parameter to specify the processor
name:
> // only set the node name
> #count(Named.as("processorName"));
>
> // only
Hi Matthias,
The reason for overloading the methods with Materialized parameter is
regarding the semantic of this class.
The Materialized class allow to name a queryable store. if a name is set
then it will be used both to name the state-store and the changelog-topic.
If no name is given, then
Just catching up on this KIP again.
One nit. The KIP says:
> In addition, the generated names have a few disadvantages to guarantee
> topology compatibilities. In fact, adding a new operator, using a
> third-library doing some optimization to remove some operators or upgrading
> to a new
Hi Guozhang,
I have updated the PR as well as the KIP. I should add more unit tests to
covers all new methods.
However, I still have one test in failure. The reason is that using
Joined.name() in both potential repartition topic and processor nodes leads
to topology-incompatible.
How should we
Hello Florian,
Just checking if have read about my previous email and if you feel happy
about it. We have the 2.2 KIP freeze deadline at 24th this month, while the
PR itself is getting quite close. So it'll be great if we can get the
agreement on it and get it into 2.2.0 release.
Guozhang
On
Hi Florian / John,
Just wanted to throw a couple minor thoughts on the current proposal:
1) Regarding the interface / function name, I'd propose we call the
interface `NamedOperation` which would be implemented by Produced /
Consumed / Printed / Joined / Grouped / Suppressed (note I
Hello Florian,
Really appreciate you for your patience.
I know that we've discussed about the approach to adding overloaded
functions and rejected it early on. But looking deeper into the current PR
I realized that this approach has a danger of great API confusions to users
(I tried to explain
Hi Florian,
Sorry about the run-around of rejecting the original proposal,
only to return to it later on. Hopefully, it's more encouraging
than frustrating that we're coming around to your initial way of
thinking.
Thanks!
-John
On Thu, Dec 13, 2018 at 4:28 PM Florian Hussonnois
wrote:
> Hi
Hi all,
Thanks again. I agree with your propositions.
Also IMHO, overloading all methods (filter, map) to accept a new control
object seems to provide a more natural development experience for users.
Actually, this was the first proposition for this KIP, but we have rejected
it because this
Hi again, all,
Matthias, I agree with you.
Florian, thanks for your response.
I think your proposal is the best way to address the ask for hiding the
name() getter. But I'd like to question that ask and instead propose that
we just make the name() getter part of the public API.
The desire to
Just catching up on this discussion.
My overall personal take is, that I am not a big fan of the interface
`Named` that is used as a factory. I would rather prefer to add a
control object parameter to all methods that don't have one yet. This
KIP was started a while ago, and we added new naming
Thank you very much for your feedbacks.
Currently, there is still lot of discussions regarding the Named interface.
On the one hand we should provided consistency over the stream API and on
the other hand we should not break the semantic as John point it up.
Guozhang, I'm sorry, but I'm little
I had one meta comment on the PR:
https://github.com/apache/kafka/pull/5909#discussion_r240447153
On Mon, Dec 10, 2018 at 5:22 PM John Roesler wrote:
> Hi Florian,
>
> I hope it's ok if I ask a few questions at this late stage...
>
> Comment 1 ==
>
> It seems like the proposal is to add a
Hi Florian,
I hope it's ok if I ask a few questions at this late stage...
Comment 1 ==
It seems like the proposal is to add a new "Named" interface that is
intended to be mixed in with the existing API objects at various points.
Just to preface some of my comments, it looks like your KIP
Hi Florian,
I've made a pass over the PR. There are some comments that are related to
the function names which may be affecting the KIP wiki page, but overall I
think it looks good already.
Guozhang
On Fri, Nov 16, 2018 at 4:21 PM Guozhang Wang wrote:
> Thanks Florian! I will take a look at
Thanks Florian! I will take a look at the PR.
On Mon, Nov 12, 2018 at 2:44 PM Florian Hussonnois
wrote:
> Hi Matthias,
>
> Sorry I was absent for a while. I have started a new PR for this KIP. It is
> still in progress for now. I'm working on it.
> https://github.com/apache/kafka/pull/5909
>
Hi Matthias,
Sorry I was absent for a while. I have started a new PR for this KIP. It is
still in progress for now. I'm working on it.
https://github.com/apache/kafka/pull/5909
Le ven. 19 oct. 2018 à 20:13, Matthias J. Sax a
écrit :
> What is the status of this KIP?
>
> -Matthias
>
> On
What is the status of this KIP?
-Matthias
On 7/19/18 5:17 PM, Guozhang Wang wrote:
> Hello Florian,
>
> Sorry for being late... Found myself keep apologizing for late replies
> these days. But I do want to push this KIP's progress forward as I see it
> very important and helpful feature for
Hello Florian,
Sorry for being late... Found myself keep apologizing for late replies
these days. But I do want to push this KIP's progress forward as I see it
very important and helpful feature for extensibility.
About the exceptions, I've gone through them and hopefully it is an
exhaustive
Hi,
The option #3 seems to be a good alternative and I find the API more
elegant (thanks John).
But, we still have the need to overload some methods either because they do
not accept an action instance or because they are translated to multiple
processors.
For example, this is the case for
Hi folks, just to summarize the options we have so far:
1) Add a new "as" for KTable / KStream, plus adding new fields for
operators-returns-void control objects (the current wiki's proposal).
Pros: no more overloads.
Cons: a bit departing with the current high-level API design of the DSL,
plus,
Hi Florian,
Sorry I'm late to the party, but I missed the message originally.
Regarding the names, it's probably a good idea to stick to the same
character set we're currently using: letters, numbers, and hyphens. The
names are used in Kafka topics, files and folders, and RocksDB databases,
and
Hi, thank you very much for all you suggestions. I've started to update the
KIP (
https://cwiki.apache.org/confluence/display/KAFKA/KIP-307%3A+Allow+to+define+custom+processor+names+with+KStreams+DSL
).
Also, I propose to rename the Processed class into Described - this will be
more meaningful
Just catching up on this thread.
I like the general idea. Couple of comments:
- I think that adding `Processed` (or maybe a different name?) is a
valid proposal for stateless operators that only have a single overload
atm. It would align with the overall API design.
- for all methods with
Hi Florian,
Re 1: I think changing the KStreamImpl / KTableImpl to allow modifying the
processor name after the operator is fine as long as we do the check again
when modifying that. In fact, we are having some topology optimization
going on which may modify processor names in the final topology
Hi ,
Thank you very much for your feedback.
1/
I agree that overloading most of the methods with a Processed is not ideal.
I've started modifying the KStream API and I got to the same conclusion.
Also ading a new method directly to KStreamImpl and KTableImpl classes
seems to be a better option.
Hi Florian,
Thanks for the KIP. What about KTable and other DSL interfaces? Will they
not want to be able to do the same thing?
It would be good to see a complete set of the public API changes.
Cheers,
Damian
On Wed, 30 May 2018 at 19:45 Guozhang Wang wrote:
> Hello Florian,
>
> Thanks for
Hello Florian,
Thanks for the KIP. I have some meta feedbacks on the proposal:
1. You mentioned that this `Processed` object will be added to a new
overloaded variant of all the stateless operators, what about the stateful
operators? Would like to hear your opinions if you have thought about
Hi Florian,
Thanks for the KIP. I think being able to add more context to the
processor names would be useful.
I like the idea of adding a "withProcessorName" to Produced, Consumed and
Joined.
But instead of adding the "Processed" parameter to a large percentage of
the methods, which would
Hi,
I would like to start a new discussion on following KIP :
https://cwiki.apache.org/confluence/display/KAFKA/KIP-307%3A+Allow+to+define+custom+processor+names+with+KStreams+DSL
This is still a draft.
Looking forward for your feedback.
--
Florian HUSSONNOIS
50 matches
Mail list logo
