[DISCUSS] Pulsar Maven Builds w/ Unit Tests
Hi - I challenge the dev community about the state of building these branches 2.11, 3,0, and Master when you actually perform all of the unit tests. In the website it seems that we always tell everyone to use `mvn clean install -DskipTests` we do this because tests take too long and are flaky. We end up counting on GitHub Workflows and must not cover all of the Unit Tests. So, if you do `mvn clean install` on any of those three branches it is very likely to fail and often in the pulsar-broker tests. What are we going to do about this? Also, pulsar.apache.org/contribute/setup-building should likely include a discussion about installing docker, Best, Dave
Re: [DISCUSS] Cherry-pick PR-16059 to 2.10 to prevent infinite unloading
Hi dev, I think we also need to consider the namespace anit-affinity-group logic too. These logics seem to do similar things. https://pulsar.apache.org/docs/3.0.x/administration-load-balance/#distribute-anti-affinity-namespaces-across-failure-domains PengHui We got three biding votes here. Do you think we should proceed to cherry-pick the PR to 2.10, then? Thanks, Heesung On Sun, Jul 2, 2023 at 5:22 PM PengHui Li wrote: > > `removeMostServicingBrokersForNamespace ` is introduced by [1] to > solve the problem that when all bundles in a particular namespace > belong to 1 or few machines, customers owning that namespace will be > heavily impacted if that broker goes down. Of course, this PR caused > the infinite unloading issue and we need to fix it. > > Thanks for the context. > It looks like we can also try to fix the infinite unloading issue. > Now, the broker is unloading the bundles without checking the distribution > of the bundles under a namespace, but it will check when finding > a new owner. Is it possible to check the bundle distribution before > unloading the bundles to avoid infinite unloading? > > Regards, > Penghui > > > On Sun, Jul 2, 2023 at 3:28 PM Enrico Olivelli > wrote: > > > +1 > > > > Enrico > > > > Il Dom 2 Lug 2023, 06:19 Hang Chen ha scritto: > > > > > +1 for cherry-picking it to branch-2.10. We have a flag to control > > > whether to enable or disable it. > > > > > > `removeMostServicingBrokersForNamespace ` is introduced by [1] to > > > solve the problem that when all bundles in a particular namespace > > > belong to 1 or few machines, customers owning that namespace will be > > > heavily impacted if that broker goes down. Of course, this PR caused > > > the infinite unloading issue and we need to fix it. > > > > > > > I agree with making it false for the next major version release by > > > default. > > > We'd better remove the config in the next version instead of change > > > the default value to `false`, which will make Pulsar's configuration > > > keep increasing. > > > > > > Thanks, > > > Hang > > > > > > [1] https://github.com/apache/pulsar/pull/388 > > > > > > PengHui Li 于2023年7月1日周六 09:38写道： > > > > > > > > +1 for cherry-pick to branch-2.10 since users don't have a workaround > > > > for this issue, and the change is well-understand, low risk. > > > > > > > > I agree with making it false for the next major version release by > > > default. > > > > > > > > Thanks, > > > > Penghui > > > > > > > > On Sat, Jul 1, 2023 at 9:26 AM Heesung Sohn > > > > wrote: > > > > > > > > > Hi dev, > > > > > > > > > > I realized that `removeMostServicingBrokersForNamespace` func in > the > > > broker > > > > > selection logic can cause infinite unloading. > > > > > > > > > > Suppose an overloaded broker unloaded a bundle and only has the > > minimum > > > > > number of bundles(in that namespace) among brokers. In that case, > the > > > > > selection logic (`removeMostServicingBrokersForNamespace`) will > > filter > > > out > > > > > other brokers and always reassign the bundle to the previous > broker. > > > This > > > > > will cause infinite unloading(like a boomerang). > > > > > > > > > > To mitigate this issue, we need to cherry-pick this PR to disable > > this > > > > > logic by the config. > > > > > https://github.com/apache/pulsar/pull/16059 > > > > > > > > > > And we probably want to disable this > > > > > `removeMostServicingBrokersForNamespace` logic by default. > > > > > > > > > > Regards, > > > > > Heesung > > > > > > > > > > >
Re: Failover Subscription - consumer assignment logic discussion
Hi Penghui and Girish, Thanks for your discussions and technical guidance! I've optimized the docs in https://github.com/apache/pulsar-site/pull/633, could you PTAL? Anyone else who wants to take a look and comment, feel free! Thanks! Yu On Tue, Jul 4, 2023 at 8:31 AM PengHui Li wrote: > > I was also thinking that from a user perspective, one would think that a > lower priority consumer is meant for backup in case one active consumer > goes down - which also doesn't work like that. > > Yes, it makes sense. We will try to think more about that to find > a solution to make Failover subscription better. I still have no idea > for now. A coordinator can help but it will introduce complexity to > Pulsar. > > Regards, > Penghui > > > On Mon, Jul 3, 2023 at 11:20 PM Girish Sharma > wrote: > > > Hello PengHui, > > > > On Mon, Jul 3, 2023 at 8:39 PM PengHui Li wrote: > > > > > > > > Got it, for the Failover subscription, the new consumer caused the > active > > > consumer > > > shift. I think we can make some improvements to this part to make sure > > the > > > new active > > > consumer will only get messages after the previous active consumer > acked > > > all the received > > > message unless the previous active consumer disconnected. > > > > > > I think this will greatly help maintain the ordering guarantees per > > partition. > > > > > > > > > > If all the consumers with the highest priority are disconnected, then > > > the consumers with a lower priority will be peeked. The Shared > > subscription > > > have different behavior. It will select the lower priority consumer if > > all > > > highest > > > priority consumers don't have available permits. I think the challenge > > for > > > Failover subscription is the broker needs to shift the active consumer > > > according > > > to the available permits. But it could be considered in a different > > active > > > consumer assigner implementation like Kafka's consumer group > coordinator, > > > you can have different policies. > > > > > > Right, in case of shared subs, the lower priority consumers are used > more > > often since permits are considered and thus, slow consumers are detected > > quickly. > > > > In Failover, the current logic can lead to a single remaining active > > consumer consuming from all partitions, while multiple lower priority > > consumers are on standby. That single higher priority consumer may not be > > able to keep up with the topic throughput. > > We cannot also directly use the same behavior as Shared subscription here > > because that would again lead to out of order delivery of messages. > > > > I do not have a solution in mind here right now but I will come up with > > something so that the load balancing can be better, utilizing lower > > priority consumers as well. > > > > I was also thinking that from a user perspective, one would think that a > > lower priority consumer is meant for backup in case one active consumer > > goes down - which also doesn't work like that. > > > > Regards > > > > > > > > > Regards, > > > Penghui > > > > > > On Mon, Jul 3, 2023 at 7:52 PM Girish Sharma > > > wrote: > > > > > > > Hello PengHui, > > > > Thank you for the reply. Adding comments inline below with a few > > > concerns. > > > > > > > > On Mon, Jul 3, 2023 at 4:38 PM PengHui Li > wrote: > > > > > > > > > Hi Girish, > > > > > > > > > > Thanks for raising the discussion. > > > > > > > > > > I can confirm that your understanding is correct, and the document > > > > > is confusing. If there are four consumers connected to a > partitioned > > > > topic > > > > > with two partitions, each partition will have four connected > > consumers > > > > but > > > > > only one active consumer. The document said two consumers are > > connected > > > > > to each partition is wrong. We will try to improve the document, > and > > > your > > > > > contribution is welcome if you want to improve it. > > > > > > > > > > Yes, the part where it shows only 2 consumers are connected is > > > > misleading, > > > > but from information point of view, it is still okay to show only 2 > in > > > the > > > > visualization, as one is active and other one is backup (next in > line) > > > > > > > > The confusion comes where it tries to indicate that the active > > consumers > > > > are uniformly spread. i.e. in the example, consumers A and C are > active > > > > while in reality, consumers A and B are active. > > > > Maybe there is scope of visualization improvement there. > > > > > > > > > > > > > > > > > For the consumer shift for the partition without active consumer > > > > failures. > > > > > I think it should be a load-balance consideration. Kafka has a > > consumer > > > > > group coordinator, which can balance traffic between consumers. But > > > > Pulsar > > > > > doesn't have. So Pulsar has to re-assign the active consumer when > the > > > > > consumer > > > > > leaves, no matter whether the consumer is active or not. > > > > > > > > > > > > > From a code
Re: [VOTE] PIP-280 : Refactor CLI Argument Parsing Logic for Measurement Units using JCommander's custom converter
+1 (binding) Thanks, Yunze On Fri, Jul 7, 2023 at 5:25 PM Joo Hyuk Kim wrote: > > Hi community, > > This PIP has received a couple of approvals in github PR link [1] > So I thought it's time to vote. > > ## Motivation > > In the current Pulsar codebase, the logic to parse CLI arguments for > measurement units like time and bytes is > > scattered across various CLI classes. Each value read has its distinct > parsing implementation, leading to a lack of code > > reuse. > > > ## Goals > > > This PIP is to refactor the argument parsing logic to leverage the > `@Parameter.converter` > > functionality provided by JCommander [link 3]. This will isolate the > measurement-specific parsing logic and increase > > code > > reusability. > > > ### In Scope > > > - Refactor all `Cmd` classes to utilize the converter functionality of > JCommander. This will streamline the parsing > > logic and simplify the codebase. > > - Refer to bottom section "Concrete Example", before "Links" > > - Or on-going PR with small use case in > https://github.com/apache/pulsar/pull/20663 > > > ## links > > > [1] PR : https://github.com/apache/pulsar/pull/20691 > > > > Best regards, > > JooHyukKim (Vince)
Re: [VOTE] Pulsar Node.js Client Release 1.9.0 Candidate 1
+1 (binding) - Verified checksum and signature - Build from source on macOS m1 with Node.js v20.4.0 - Run basic e2e examples Two additional notes: 1. You should use https://downloads.apache.org/pulsar/KEYS instead of https://dist.apache.org/repos/dist/dev/pulsar/KEYS 2. When building from the TAR source, you need to modify `pkg/mac/common.sh`. Change `ROOT_DIR=$(git rev-parse --show-toplevel)` to `ROOT_DIR=$PWD`. Thanks, Yunze On Fri, Jul 7, 2023 at 10:28 AM Masahiro Sakamoto wrote: > > +1 (binding) > > - verified checksum and signature > - confirmed that the build was successful > - ran produce/consume > > Regards, > > Masahiro Sakamoto > Yahoo Japan Corp. > E-mail: massa...@yahoo-corp.jp
[VOTE] PIP-280 : Refactor CLI Argument Parsing Logic for Measurement Units using JCommander's custom converter
Hi community, This PIP has received a couple of approvals in github PR link [1] So I thought it's time to vote. ## Motivation In the current Pulsar codebase, the logic to parse CLI arguments for measurement units like time and bytes is scattered across various CLI classes. Each value read has its distinct parsing implementation, leading to a lack of code reuse. ## Goals This PIP is to refactor the argument parsing logic to leverage the `@Parameter.converter` functionality provided by JCommander [link 3]. This will isolate the measurement-specific parsing logic and increase code reusability. ### In Scope - Refactor all `Cmd` classes to utilize the converter functionality of JCommander. This will streamline the parsing logic and simplify the codebase. - Refer to bottom section "Concrete Example", before "Links" - Or on-going PR with small use case in https://github.com/apache/pulsar/pull/20663 ## links [1] PR : https://github.com/apache/pulsar/pull/20691 Best regards, JooHyukKim (Vince)
[DISCUSS] Move superuser/tenantAdmin checks to AuthorizationService from AuthorizationProvider
Hello everyone, When a role wants to use the resource, the role needs to have resource permissions. The process is to first check whether the role is the superuser or tenant administrator. If yes, operations are allowed. Otherwise, check the policies stored in zk. Right now, we have the AuthorizationService and AuthorizationProvider, the AuthorizationService wraps the AuthorizationProvider call. When you check the code, you will find that both classes have the superuser/tenantAdmin checks in certain places, this may cause confusion when developing the custom AuthorizationProvider, so I suggest unifying superuser/tenantAdmin checks in the `AuthorizationService`, and then the `AuthorizationProvider` only needs to consider their business permissions. I created a PR a while ago, you can check it out: https://github.com/apache/pulsar/pull/20145, Thanks, Zixuan
