> we can allow adding new "public" methods as you mentioned, but only if this doesn't change the semantics of an interface.
However, I think we should not allow introducing a new method to the interface that is possibly used by the user to the patch release. For instance, If a user is using a new method introduced in 3.0.1 to develop the pulsar connector, then the connector cannot be used in 3.0.0 due to compatibility. That means the user can't downgrade the pulsar to 3.0.0. I think it's unacceptable. --- The ecosystem module uses too many APIs that should be considered as the internal API. For instance, the pulsar-broker module should be considered as the internal module. But the ecosystem module still relies on it. Therefore I'm +1 for continuing the support of the Interface Taxonomy. This sounds like a good solution. But for the current situation, too many APIs should be annotated according to the PIP-72. To unblock the release of 3.0.1, I propose that if there are new APIs that need to be cherry-picked to the patch release to fix the bug, we need to apply the Taxonomy annotations for them first. Especially for the `InterfaceAudience`. And we should not add any new `Audience.Public` API to the patch release. BR, Zike Yang On Mon, Jul 17, 2023 at 9:00 AM PengHui Li <peng...@apache.org> wrote: > > Hi Yunze, > > Thanks for raising the discussion. > > IMO, The major issue is we don't have a clear API definition for the > pulsar-broker module like > pulsar-client-api and pulsar-client-admin-api modules. We have a > clear Interface Taxonomy[0] for > pulsar-client-api and pulsar-client-admin-api modules. I don't think it's > an uncleared compatibility > guarantee issue. It is also defined in Interface Taxonomy[0], and I agree > it should be shown in the > Pulsar's documentation instead of a PIP. > > Without the Interface Taxonomy[0], we are not able to distinguish whether > the public method is > only for Pulsar internal or users. Maintaining the compatibility for all > the public methods for the > pulsar-broker module is obviously more burdensome for the internal "public > methods". > The proposal[0] already has a good explanation for this part (Internal, > Public) > > Asaf also shared the solution of the OpenTelemetry project with me. They > use a different > package name like > `io.opentelemetry.sdk.metrics.internal.aggregator`. Essentially, it should > be the same > idea as Interface Taxonomy[0], providing a way for users to know which > interfaces/methods should use or not use. > But it's hard for Pulsar to apply the package name solution because it will > introduce a huge break change. > I support continuing the Interface Taxonomy[0] solution, and starting to > add the annotation to the pulsar-broker module. > We can also have a CI to enforce the binary compatibility for any classes > with Interface Taxonomy[0] annotations. > > Regards, > Penghui > > [0] > https://github.com/apache/pulsar/wiki/PIP-72%3A-Introduce-Pulsar-Interface-Taxonomy%3A-Audience-and-Stability-Classification > > > > On Fri, Jul 14, 2023 at 10:59 PM Yunze Xu <x...@apache.org> wrote: > > > Hi Enrico, > > > > > Also adding it to all the existing classes is a big task. > > > > Yes. I don't support adding it to existing classes as well. But we > > need to apply the annotation for new code. > > > > > because basically we wouldn't be able to commit change if we prevent > > adding/removing methods that are marked "public" > > > > Yes. So I proposed that it's allowed to add any public method if > > necessary, but it's not allowed to remove any public method directly. > > I don't think it's impossible. The worst case is that there are many > > deprecated classes or methods, but the compatibility is guaranteed and > > they can be safely removed in the next minor release. > > > > Hi Lari, > > > > > The WebSocketPingPongServlet and PingPongHandler never made any sense. > > > > I agree. I just share an example to assume the code is checked without > > any further concern. For a more loose solution, we can require any PR > > that removes or changes the public method signature to explain it in > > detail. > > > > Thanks, > > Yunze > > > > > > > > On Fri, Jul 14, 2023 at 8:31 PM Lari Hotari <l...@hotari.net> wrote: > > > > > > Commenting on bringing up the example of PR > > > https://github.com/apache/pulsar/pull/20733 . > > > It isn't a good example of breaking compatibility at all. > > > > > > The WebSocketPingPongServlet and PingPongHandler never made any sense. > > > The issue eclipse/jetty.project#4880 [1] explains that Websocket Pong > > > is provided out-of-the-box in Jetty and cannot be disabled. > > > Based on this information, it seems that Pong logic has always been > > available. > > > There was never a need to add WebSocketPingPongServlet and > > > PingPongHandler in the first place. > > > > > > -Lari > > > > > > 1 - https://github.com/eclipse/jetty.project/issues/4880 > > > > > > On Fri, Jul 14, 2023 at 2:13 PM Yunze Xu <x...@apache.org> wrote: > > > > > > > > In addition to the rule of the patch releases, it's acceptable to have > > > > new public methods for bug fixes, though we should pay enough effort > > > > to avoid it. If so, we should not remove or change any public method. > > > > > > > > I just checked a few recent PRs in branch-3.0 and added some examples > > here. > > > > > > > > https://github.com/apache/pulsar/pull/20733 Instead of removing > > > > WebSocketPingPongServlet and PingPongHandler, we should mark them as > > > > deprecated once we agree to my proposal here. > > > > > > > > https://github.com/apache/pulsar/pull/20666 It's not acceptable even > > > > for now because it changes the public API > > > > org.apache.pulsar.broker.loadbalance.extensions.filter.BrokerFilter. > > > > It's also a bad example. > > > > > > > > https://github.com/apache/pulsar/pull/20677 It's not acceptable that > > > > the static `TopicCompactionStrategy#load` method signature changed. > > > > > > > > #20666 and #20677 are also bad examples that the public APIs are not > > > > designed well in the PIP-192 proposal. The methods are not designed > > > > well so the author modify them casually. > > > > > > > > Thanks, > > > > Yunze > > > > > > > > On Fri, Jul 14, 2023 at 6:52 PM Yunze Xu <x...@apache.org> wrote: > > > > > > > > > > Hi all, > > > > > > > > > > As the Pulsar community is growing, the core project could be > > depended > > > > > on by many ecosystem projects, including protocol handlers and > > > > > connectors. However, there is no clear API compatibility guarantee at > > > > > the moment. > > > > > > > > > > For now, PIP is required when public APIs change [1]. However, I > > think > > > > > the public API only refers to the interfaces in: > > > > > - The pulsar-client-api module > > > > > - The pulsar-admin-api module > > > > > - Any plugin interface, e.g. ProtocolHandler > > > > > > > > > > However, when a 3rd party project adds Pulsar as the dependency, it's > > > > > legal and common to use any public class and any public method from > > > > > it. For a specific example, just as I've discussed many times, the > > > > > MessageId interface from the pulsar-client-api module, the 3rd party > > > > > project tends to cast it to the specific implementation (e.g. > > > > > MessageIdImpl) to use.Then the challenge comes, if we modifies any > > > > > public method of MessageIdImpl, the ecosystem developer could suffer > > > > > from it. > > > > > > > > > > For this case, we have no clear rule on how Pulsar guarantees the API > > > > > compatibility. So I'd like to propose my suggestion here, once it > > > > > reaches the consensus, I will write a formal proposal and add it to > > > > > our contribution guide [2]. > > > > > > > > > > Here are my proposed rules (I will add specific examples for a > > formal proposal): > > > > > 1. For any new interface, add the > > > > > org.apache.pulsar.common.classification.InterfaceStability > > > > > 2. For patch releases, e.g. 3.0.x to 3.0.y, we should never introduce > > > > > any public method change. > > > > > 3. For minor releases, e.g. 3.1.0 to 3.2.0, when you want to modify a > > > > > public method, e.g. `foo(int, int)` is changed to `foo(int, String)`, > > > > > don't remove the original `foo(int, int)` method. Instead, add the > > > > > `@Deprecated` annotation and keep the semantics not changed if > > > > > possible. > > > > > 4. Any public method with the `@Deprecated` annotation can be removed > > > > > in the next minor release. > > > > > 5. The rules above are applied to all modules, not only the xxx-api > > modules. > > > > > > > > > > With the rules above, > > > > > - For ecosystem developers, they can know the risk to use any public > > > > > method from a class, especially implementation class. They can also > > be > > > > > confident that no API will change if they upgrade Pulsar from a.b.c > > to > > > > > a.b.d. > > > > > - For committers, they should be more careful to review the PRs that > > > > > introduce API changes. > > > > > - For release managers, it would be clear that any PR that changes > > any > > > > > public method should not be cherry-picked to release branches. > > > > > > > > > > [1] > > https://github.com/apache/pulsar/tree/master/pip#when-is-a-pip-required > > > > > [2] https://pulsar.apache.org/contribute/ > > > > > > > > > > Thanks, > > > > > Yunze > >
