Skip to content

Conversation

@milleruntime
Copy link
Contributor

This reverts commit 2e8144e.

The move of this class breaks API compatibility in the class hierarchy. I am not sure how the parent class being in the clientImpl is not picked up by the apilyzer plugin. It could be since the super class of AbstractLexicoder in clientImpl happens to be in the public API that it doesn't actually expose any internal class. While it is awkward that AbstractLexicoder is in a different non-API package, it seems that it ultimately doesn't matter for the public API.

Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were we able to confirm that this actually breaks compatibility? I believe it only would affect things if the user actually assigned it to a variable of the type of the superclass. That'd be okay to break, since the superclass wasn't public API. I would like to see that tested and confirmed before we even consider any reverting.

Also, I don't know where the decision was made that binary incompatibility is unacceptable in a minor release. I see that on the website at https://accumulo.apache.org/api , but I don't recall a discussion to decide that. I think it'd be good to question whether we really want to offer this as a guarantee.

And finally, even if there is a binary breakage here, and we think it is a violation, it does not necessarily mean we have to rename the release to 3.0.0. We can still keep the 2.1 name. SemVer rules are guidelines only. There's no reason we can't document the occasional exception.

I think it is extremely important to stop exposing internal APIs in our public APIs. We've worked really hard to complete this transition. This would be a step backwards, and I do not want to see this reverted, to re-entangle an internal API with our public APIs. So, I'm strongly against this change. If we can confirm this is definitely breaking binary compatibility, I would prefer we 1) question whether we should even be guaranteeing that across minor versions in the first place, and 2) question whether that's more important than leaving it in the API later only to have more users affected later. If we have to leave a break and don't want to bump to the major version, I'd rather just document it thoroughly as a breakage that was necessary to accomplish the goal of keeping the internals unexposed in the public API.

@milleruntime
Copy link
Contributor Author

Were we able to confirm that this actually breaks compatibility?

The only breakage I was able to reproduce was a "cannot find symbol" error since the class was moved. But this is easily fixed by changing the import. And this is with the import of the class from a "clientImpl" package.

I am not convinced this needs to be reverted. I only opened this PR in reference to the Slack conversation and your findings in https://people.apache.org/~ctubbsii/compliance/2.0-2.1.html.

@milleruntime
Copy link
Contributor Author

Closing since I don't think we want to do this revert.

@milleruntime milleruntime deleted the revert-lexicoder branch November 4, 2021 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants