Yunze, I agree overall with the proposal, I added some comments inline below
Enrico Il giorno ven 14 lug 2023 alle ore 13:25 Yunze Xu <x...@apache.org> ha scritto: > > Good suggestion. We can try that. > > Thanks, > Yunze > > On Fri, Jul 14, 2023 at 7:03 PM Lari Hotari <lhot...@apache.org> wrote: > > > > Makes sense. We should use tooling in CI to enforce binary > > compatibility of the modules that shouldn't break. > > There's japicmp [1] that can be used to do this. japicmp comes with a > > maven plugin [2] that needs to be configured with the rules to be > > used. > > > > -Lari > > > > 1 - https://siom79.github.io/japicmp/ > > 2 - https://siom79.github.io/japicmp/MavenPlugin.html > > > > On Fri, Jul 14, 2023 at 1:55 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 I don't know how feasible this is, but we can try. It is up to the people who contribute the code to remember to add this and who commits the patch. Also adding it to all the existing classes is a big task. > > > 2. For patch releases, e.g. 3.0.x to 3.0.y, we should never introduce > > > any public method change. +1 we can allow adding new "public" methods as you mentioned, but only if this doesn't change the semantics of an interface. For instance adding a method to a Listener interface like this interface ListenerV1 { onNew(event) // receive events when something is changed or deleted onChange(event) } to interface ListenerV2 { onEvent(Event) onNew(event) // receive events when something is changed onChange(event) // receive events when something is deleted onDelete(event) } this would be an incompatible change because the behaviour of "onChange" changed > > > 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. > > > Unfortunately it is hard to say "which modules" are public/interesting for our audience. This should be strictly applied to the very public API (clients, REST API....), but we cannot ensure that we do it for every module, because basically we wouldn't be able to commit change if we prevent adding/removing methods that are marked "public" in Java, but that actually are not part of the "public" surface of Pulsar. Unfortunately we have opened the internals of the broker to many "extension" points and it is hard to say what we can change and what not Enrico > > > 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
