+1 (binding)
Enrico
Il giorno mar 9 mag 2023 alle ore 08:10 Yunze Xu <x...@apache.org> ha scritto:
>
> +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
> > >>
> > >
