Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] o.o.a.s.m.AcknowledgedResponse is deprecated without an alternative class #14665

Closed
dbwiddis opened this issue Jul 6, 2024 · 3 comments
Labels
backwards-compatibility bug Something isn't working Cluster Manager deprecate Libraries Lucene Upgrades and Libraries, Any 3rd party library that Core depends on, ex: nebula; team is respo

Comments

@dbwiddis
Copy link
Member

dbwiddis commented Jul 6, 2024

Describe the bug

The o.o.a.s.m.AcknowledgedResponse class, returned as an ActionListener response from many OpenSearch APIs, produces deprecated warnings (required by JLS 9.6.3.6). Nothing in the class itself indicates any alternative, as strongly recommended by Java documentation:

You are strongly recommended to use the Javadoc @deprecated tag with appropriate comments explaining how to use the new API. This ensures developers will have a workable migration path from the old API to the new API.

The class itself isn't deprecated, the entire package in which it resides is, to support inclusive language.

This creates an annoyance to developers who find themselves with a compiler warning that they have no alternative to, and often choose to use @SuppressWarnings("deprecation") to avoid the warnings (and yellow lines/icons in IDEs); sometimes suppressing an entire class if there are many uses of this type. This has a bad side-effect of potentially missing other deprecated classes.

Related component

Libraries

To Reproduce

  1. Use an API implementation method such as IndicesAdminClient.delete() that returns an AcknowledgedResponse.
  2. Observe a yellow-squiggly underline under the class name.

Expected behavior

Deprecated classes use the @deprecated annotation to tell developers what alternative they should use, as strongly recommended by Java documentation.

While many classes in the deprecated package have alternatives, AcknowledgedResponse does not.

The AcknowledgedRequest and ShardsAcknowledgedResponse classes have similar issues but are not nearly as ubiquitous.

Additional Details

Screenshots

Screenshot 2024-07-06 at 2 37 52 PM

Suggested Fix

I'm happy to submit a PR to fix this if other maintainers agree on the best approach.

I think it's reasonably easy to make a copy of the existing classes in the new inclusively-named package, and then change the existing classes to be subclasses of the non-deprecated ones. That would maintain compatibility for existing code, while providing a migration package for developers who, even after suppressing warnings, still have non-inclusive language in their import statements.

Alternative

The package-level deprecation could be removed in favor of individual class-level deprecation, if there is no actual intent to ever migrate the AcknowledgedResponse to another package.

@dbwiddis dbwiddis added bug Something isn't working untriaged labels Jul 6, 2024
@github-actions github-actions bot added the Libraries Lucene Upgrades and Libraries, Any 3rd party library that Core depends on, ex: nebula; team is respo label Jul 6, 2024
@peternied
Copy link
Member

[Triage - attendees 1 2 3 4 5]
@dbwiddis Thanks for creating this issue, look forward to seeing the resolution

@dbwiddis
Copy link
Member Author

@peternied Spent a bit of time trying to address this, but it's not as easy as one would expect.

Copying the classes to the new package with "clustermanager" naming and adding @deprecated tag in the old class is easy. But users aren't really in control of these types, they have to handle them from the client calls they make.

I can't just extend the new class because these classes already extend deprecated superclasses, and Java doesn't permit multiple inheritance.

Which means actually doing things means changing all 153 classes using AcknowledgedResponse to return the new type, which is a breaking change.

The simpler alternative is to delete the package-level deprecation line, since we obviously didn't deprecate every class in the package. All the classes with new alternatives have the appropriate deprecation lines.

So I'll repeat my offer to fix this but it's going to be a lot of work and I'd really like more people than you telling me vaguely that you look forward to a resolution, to actually agree that it's worth doing all this work because of the package naming.

@dbwiddis
Copy link
Member Author

Closing this as I see #4856 is tracking it
CC: @peternied

@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in Cluster Manager Project Board Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-compatibility bug Something isn't working Cluster Manager deprecate Libraries Lucene Upgrades and Libraries, Any 3rd party library that Core depends on, ex: nebula; team is respo
Projects
Status: ✅ Done
Development

No branches or pull requests

2 participants