Hello Yishun,
I looked further into the change needed on NetworkClient, comparing them
with the changes on AdminClient / ConsumerNetworkClient, and I think it is
indeed a quite large change needed on the latter two classes if we do not
want to touch on NetworkClient.
Colin's argument against havi
@Guozhang Wang What do you think?
On Fri, Sep 14, 2018 at 2:39 PM Yishun Guan wrote:
>
> Hi All,
>
> After looking into AdminClient.java and ConsumerClient.java, following
> the original idea, I think some type specific codes are unavoidable
> (we can have a enum class that contain a list of batch
Hi All,
After looking into AdminClient.java and ConsumerClient.java, following
the original idea, I think some type specific codes are unavoidable
(we can have a enum class that contain a list of batch-enabled APIs).
As the compatibility codes that breaks down the batch, we need to
either map one
Hi Collin and Guozhang,
I see. But even if the fall-back logic goes into AdminClient and
ConsumerClient, it still have to be somehow type specific right?
So either way, there will be api-specific process code somewhere?
Thanks,
Yishun
On Tue, Sep 4, 2018 at 5:46 PM Colin McCabe wrote:
>
> Hi Y
Hi Yishun,
I agree with Guozhang. NetworkClient is the wrong place to put things which
are specific to a particular message type. NetworkClient should not have to
care what the type of the message is that it is sending.
Adding type-specific handling is much more "ugly and dirty" than adding s
Hello Yishun,
I reviewed the latest wiki page, and noticed that the special handling
logic needs to be in the NetworkClient.
Comparing it with another alternative way, i.e. we add the fall-back logic
in the AdminClient, as well as in the ConsumerClient to capture the
UnsupportedException and fall
Hi Guozhang,
Yes, I totally agree with you. Like I said, I think it is an overkill
for now, to make so many changes for a small improvement.
And like you said, the only way to do this is the "ugly and dirty"
way, do you think we should still apply this improvement?
I updated a new BuildToList() (
Hi Yishun,
I was actually not suggesting we should immediately make such dramatic
change on the AbstractRequest APIs which will affect all requests types,
just clarifying if it is your intent or not, since your code snippet in the
KIP has "@Override" :)
I think an alternative way is to add such
Hi Guozhang,
Yes, you are right. I didn't notice T build() is bounded to .
I was originally thinking T could be an AbstractedRequest or List<>
but I was wrong. Now the return type has to change from T build() to
List build
where . As you mentioned,
this required a change for all the requests, prob
Hello Yishun,
Thanks for the updated KIP. I think option 1), i.e. return multiple
requests from build() call is okay. Just to clarify: you are going to
change `AbstractRequest#build(short version)` signature, and hence all
requests need to be updated accordingly, but only FindCoordinator for may
r
@Guozhang Wang Could you review this again when you have time? Thanks! -Yishun
On Wed, Aug 29, 2018 at 11:57 AM Yishun Guan wrote:
>
> Hi, because I have made some significant changes on this design, so I
> want to reopen the discussion on this KIP:
> https://cwiki.apache.org/confluence/x/CgZPBQ
>
Hi, because I have made some significant changes on this design, so I
want to reopen the discussion on this KIP:
https://cwiki.apache.org/confluence/x/CgZPBQ
Thanks,
Yishun
On Thu, Aug 16, 2018 at 5:06 PM Yishun Guan wrote:
>
> I see! Thanks!
>
> On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang wrote
I see! Thanks!
On Thu, Aug 16, 2018, 4:35 PM Guozhang Wang wrote:
> It is not implemented, but should not be hard to do so (and again you do
> NOT have to do that in this KIP, I'm bringing this up so that you can help
> thinking about the process).
>
> Quoting from Colin's comment:
>
> "
> The p
It is not implemented, but should not be hard to do so (and again you do
NOT have to do that in this KIP, I'm bringing this up so that you can help
thinking about the process).
Quoting from Colin's comment:
"
The pattern is that you would try to send a request for more than one
group, and then yo
Hi, I am looking into AdminClient.scala and AdminClient.java, and also
looking into ApiVersionRequest.java and ApiVersionResponse.java, but I
don't see anywhere contains to logic of the one-to-one mapping from version
to version, am i looking at the right place?
On Mon, Aug 13, 2018 at 1:30 PM Guo
Regarding 3): Today we do not have this logic with the existing client,
because defer the decision about the version to use (we always assume that
an new versioned request need to be down-converted to a single old
versioned request: i.e. an one-to-one mapping), but in principle, we should
be able t
@Guozhang, thank you so much!
1. I agree, fixed.
2. Added.
3. I see, that is something that I haven't think about. How does Kafka
handle other api's different version problem now? So we have a specific
convertor that convect a new version request to a old version one for each
API (is this what the
Hello Yishun,
Thanks for the proposed KIP. I made a pass over the wiki and here are some
comments:
1. "DESCRIBE_GROUPS_RESPONSE_MEMBER_V0", why we need to encode the full
schema for the "COORDINATOR_GROUPIDS_KEY_NAME" field? Note it includes a
lot of fields such as member id that is not needed fo
Hi, thank you Ted! I have addressed your comments:
1. Added more descriptions about later optimization.
2. Yes, I will implement the V3 later when this KIP gets accepted.
3. Fixed.
Thanks,
Yishun
On Fri, Aug 10, 2018 at 3:32 PM Ted Yu wrote:
> bq. this is the foundation of some later possible
bq. this is the foundation of some later possible optimizations(enable
batching in *describeConsumerGroups ...*
*Can you say more why this change lays the foundation for the future
optimizations ?*
*You mentioned **FIND_COORDINATOR_REQUEST_V3 in the wiki but I don't see it
in PR.*
*I assume you w
It would be great if I could get some feedbacks on this KIP, thanks!
On Thu, Aug 9, 2018, 10:35 AM Yishun Guan wrote:
> To add more context for KIP-347: https://github.com/apache/kafka/pull/5353
>
> On Wed, Aug 8, 2018 at 3:55 PM Yishun Guan wrote:
>
>> Hi all,
>>
>> I would like to start a dis
To add more context for KIP-347: https://github.com/apache/kafka/pull/5353
On Wed, Aug 8, 2018 at 3:55 PM Yishun Guan wrote:
> Hi all,
>
> I would like to start a discussion on:
>
> KIP-347: Enable batching in FindCoordinatorRequest
> https://cwiki.apache.org/confluence/x/CgZPBQ
>
> Thanks @Guoz
Hi all,
I would like to start a discussion on:
KIP-347: Enable batching in FindCoordinatorRequest
https://cwiki.apache.org/confluence/x/CgZPBQ
Thanks @Guozhang Wang for his help and patience!
Thanks,
Yishun
23 matches
Mail list logo
