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 >
