+1 (binding)
The separated TopicBundleAssignmentStrategy and bundleSplitListener LGTM.
Typo:
* TopicBunldeAssignmentFactory -> TopicBundleAssignmentFactory
* TopicBunldeAssignmentStrategy -> TopicBundleAssignmentStrategy
Thanks,
Yunze
On Tue, May 9, 2023 at 1:17 PM PengHui Li <peng...@apache.org> wrote:
>
> 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
> >>
> >
