Hi Lin,
I noticed the PIP is updated, and thanks for addressing my comments.
It looks good to me now.
+1 (binding)
Regards,
Penghui
On Mon, May 8, 2023 at 5:16 PM PengHui Li <peng...@apache.org> wrote:
> I'm sorry for not getting back to you sooner.
>
> From the description of the proposal. You have figured out the problems
> with the current
> solution. From my understanding, only the first one is in the scope of
> this proposal.
> The other two problems, one is about the issue from ThresholdShedder and
> another
> one is about the load data sync issue.
>
> It would be better to have a section "Scope" to underline that the
> proposal is
> trying to resolve the first issue. Or we can just describe it in the
> motivation section like
>
> ---
> There are a couple of problems with the current load balancer 1, 2, 3.
> This proposal is
> trying to resolve the first issue by introducing a pluggable topic bundle
> assigner to
> allow users to customize the topic bundle assignment according to their
> use case.
> Problem 2 and 3 is not in the scope of this proposal.
> ---
>
> Just to make it more clear for reviewers to understand what is in the
> scope and not in
> the scope.
>
> I remember there was an implementation section for assigning partitions to
> different bundles. It will be an excellent example for users to understand
> what a customized
> topic bundle assigner looks like. I think we can add it back as an example
> (not implementation)
>
> For the newly introduced configuration in broker.conf.
> I think we'd better use `topicBundleAssignmentStrategy` instead of `
> partitionAssignerClassName.`
> because it is only for partitions. Users can also implement their own
> BundleAssignmentStrategy
> for
> non-partitioned topics.
>
> For the new methods in the new strategy class:
>
> ---
> void onBundleSplit(NamespaceBundle bundle);
> void onBundleUnload(NamespaceBundle bundle);
> void onBundleOwned(NamespaceBundle namespaceBundle);
> ---
>
> If I understand correctly, there are some cases users might need to handle
> the above events
> to do some topics unloading or other operations. Is it better to
> introduce a listener interface
> to the LoadManager? So that we don't need to add the above method in the
> AssignmentStrategy
> class. For example:
>
> ---
> new MyTopicBundleAssignmentStrategy(PulsarService pulsar) {
> pulsar.getLoadManager().registerListener(...)
> }
> ---
>
> Not all TopicBundleAssignmentStrategy implementations need to handle the
> events, right?
> So that we can make the new interface better to follow the SRP (Single
> Responsibility Principle).
>
> Thanks,
> Penghui
>
> On Fri, Apr 21, 2023 at 1:50 PM linlin <lin...@apache.org> wrote:
>
>> Hi Pulsar Community,
>>
>> This thread is to start the vote for PIP 255.
>> After discussion, we believe that it is a better way to plug-in the
>> partition assignment strategy,
>> and let customers complete the specific implementation.
>>
>> So the title is change from "Assign topic partitions to bundle by round
>> robin" to
>> "Make the partition assignment strategy pluggable"
>>
>> Discussion:
>> https://lists.apache.org/thread/pxx715jrqjc219pymskz8xcz57jdmcfy
>> Issue: https://github.com/apache/pulsar/issues/19806
>>
>> Thanks,
>> Lin Lin
>>
>
