[RFC] Reconsider the breaking changes check policy

[reta]

Is your feature request related to a problem? Please describe

In the https://github.com/opensearch-project/OpenSearch/issues/9305, the very useful check has been introduced into the GA workflows to ensure that any changes in public APIs (@PublicApi) are non-breaking. However the assumptions that check follows at the moment are too strict: we compare the APIs between current changeset and the latest branch snapshot.

Describe the solution you'd like

It seems like we need to reconsider the rules by which the breaking changes are detected. First and foremost, the rules should be release driven:

• main or 3.0.0 is next major release which is allowed to have any breaking changes (needs to be documented though)
• main or 3.0.0 has never been released (it exists in SNAPSHOTs only) and technically any change is allowed here

I would propose for main to detect breaking changes against latest 2.x release and make the breaking check advisory: it should not fail the build but, as an option, add a warning comment that breaking changes were detected, needs documentation.

Now, where the breaking changes are really important is:

• backporting from 3.x to 2.x, which consequently lead to ...
• ... any changes in 2.x that are breaking with respect to latest released version

To justify why comparing with release is very important: we should acknowledge that in 2.x there could be new public APIs introduced and since these APIs are new, they could (and should be allowed to) change, in any ways. But once released, it is set in stone and breaking those is not an option.

I believe there we have to be strict and fail the build.

Related component

Build

Describe alternatives you've considered

No response

Additional context

See please https://github.com/opensearch-project/OpenSearch/pull/13244

[reta]

@peternied @dblock would very much appreciate your thoughts folks, thank you

[peternied]

@sachinpkale @gbbafna As you were both recently impacted by the ambiguity around how to 'resolve' breaking changes workflow errors what thoughts do you have?

[gbbafna]

We need to rethink about the PublicApi annotation . Lets talk about IndexShard constructor , which is although public , but kind of private to opensearch and is not meant to be exposed to plugins or opensearch contract . We will routinely add more fields to the class , the constructor and the functions. Any changes to the constructor would break the Breaking Changes check in its current form . Refer this PR for example.

Even if we try to mark it as InternalApi, the check will fail saying that it can't marked as internal . I think we need a mechanism to override the same and mark the functions, fields , classes as InternalApi . Is there a way to do that forcibly ? I do understand that it leaves us vulnerable to bad overrides, but we need to make the developer's experience better as well.

We tried to mark RemoteStoreEnums etc as well with InternalApi annotation, but the policy wouldn't allow the same.

[dblock]

I liked having the breaking change as an indicator. Is it enough to give maintainers the ability to override it with a breaking-changes tag? It would help collect those for release notes and generally call attention to more eyes that can review a breaking change, including marking existing classes such as IndexShard as @InternalApi.

[reta]

We need to rethink about the PublicApi annotation . Lets talk about IndexShard constructor , which is although public , but kind of private to opensearch and is not meant to be exposed to plugins or opensearch contract .

@gbbafna this issue could be solved in multiple ways, depending what we would have in our disposal at given moment of time. To make but kind of private to opensearch just switch constructor to package private and have a companion class that creates index shard but is not a public API (there are many alternatives). The problem is - the code and intent should match, it needs a bit more effort sometimes.

[reta]

including marking existing classes such as IndexShard as @InternalApi.

@dblock Here is the issue, we could mark everything as @InternalApi and solve check problem but not the API problem. This class (and many others) leak through plugins. The message would be something like that: "here is EnginePlugin plugin please use it, but you know what, anything could change" , not something I would be happy about as a developer to be fair.

[gbbafna]

It would help collect those for release notes and generally call attention to more eyes that can review a breaking change, including marking existing classes such as IndexShard as @InternalApi.

@dblock , we can't mark them as @InternalApi, as of now as the gradle check itself will fail. If we allow that on maintainer's judgement and not enforce , our lives will be much easier.

To make but kind of private to opensearch just switch constructor to package private and have a companion class that creates index shard but is not a public API (there are many alternatives). The problem is - the code and intent should match, it needs a bit more effort sometimes.

@reta , many times we can't mark the classes as package-private as integ tests need access to them as well.

[reta]

@reta , many times we can't mark the classes as package-private as integ tests need access to them as well.

This is solvable with https://github.com/opensearch-project/OpenSearch/issues/13275#issuecomment-2063942396 fe

[reta]

@dblock , we can't mark them as @internalapi, as of now as the gradle check itself will fail. If we allow that on maintainer's judgement and not enforce , our lives will be much easier.

This check is ours, nothing set in stone, we could modify it in any way we think it makes sense. One option fe, we could to support the @InternalApi on constructors to highlight the fact that instantiation of that particular type is not supposed to be done by outside of the OpenSearch.

The maintainer's judgement does not work - we broke so many APIs :(

[gbbafna]

@dblock , we can't mark them as @internalapi, as of now as the gradle check itself will fail. If we allow that on maintainer's judgement and not enforce , our lives will be much easier.

This check is ours, nothing set in stone, we could modify it in any way we think it makes sense. One option fe, we could to support the @InternalApi on constructors to highlight the fact that instantiation of that particular type is not supposed to be done by outside of the OpenSearch.

That would be great actually. It would solve the other problem we had of forcibly marking RemoteStoreEnum as a PublicApi as well . Let me open a GH issue for same .

[peternied]

I don't think this is 'resolved' but we are making progress (yay!). Keeping this open until we've got more of an end to end idea how this looks.

[peternied]

[Triage - attendees 1 2 3 4 5 6 7 8] @reta Thanks for creating this RFC