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
