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
