You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ctubbsii (via GitHub)" <gi...@apache.org> on 2023/12/06 20:31:13 UTC
[PR] Remove and prevent use of terminal deprecations [accumulo]
ctubbsii opened a new pull request, #4032:
URL: https://github.com/apache/accumulo/pull/4032
Avoid use of `forRemoval = true` to indicate terminally deprecated items. Although explicitly communicating that something may be removed is a nice-to-have, the justification for avoiding it is:
1. terminal deprecations are new in Java, and optional to use; ordinarily deprecated items can still be subject to removal, especially for code that was ordinarily deprecated prior to the introduction of this new forRemoval attribute; so, users cannot rely on only terminally deprecated items being removed
2. terminal deprecations behave differently than ordinary deprecations in that the use of terminally deprecated items in deprecated code still triggers a compiler warning, and the proper use of these and suppression of these warnings are difficult to use correctly or reason about (see https://docs.oracle.com/javase/specs/jls/se11/html/jls-9.html). One example is the deprecation of an interface method when the subclass is not also deprecated. This will result in a warning only if the user directly accesses the method on the implementing class instead of through the interface. For that reason, it is often recommended to deprecate subclass methods as well. However, for terminally deprecated interface methods, the mere existence of a subclass implementation that overrides the interface method is enough to trigger a warning, even if it is not used directly (it is used directly by the subclass to define the override, which is confusing). However, the most common problem is just that sin
ce terminal deprecations still trigger a warning, even when used in deprecated code, you have to add excessive warnings suppressions everywhere, even in deprecated code, to get a clean build. This can also make code very ugly, because the warnings appear even in imports, so you have to use fully-qualified class names instead of import statements (because you can't suppress warnings in import statements).
3. warnings suppressions of terminally deprecated items are not implemented properly in various IDEs, making it further difficult to use (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=565271)
4. Semantic Versioning allows for any previously deprecated item to be removed and cause a breaking change on a major release. These breaking changes are possible regardless of whether the deprecation was marked ordinarily or terminally; so the use of this is entirely unnecessary, as it does not help the user reliably predict what will be broken in a future major release
There are several options to address forRemoval:
1. Not enforce its use or non-use
2. Always use it
3. Come up with a means to decide under what circumstances it should be used and what circumstances it shouldn't be used
4. Never use it
The first option leads to confusion and inconsistency. The second option leads to problems of misuse, IDE errors, and excessive warnings suppressions. The third option is complicated, and overall, probably would be inconsistently applied and hard to enforce. So, this PR resorts to the fourth option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove and prevent use of terminal deprecations [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4032:
URL: https://github.com/apache/accumulo/pull/4032#discussion_r1418121943
##########
core/src/main/java/org/apache/accumulo/core/manager/balancer/TabletStatisticsImpl.java:
##########
@@ -46,7 +46,7 @@ public long getNumEntries() {
}
@Override
- @SuppressWarnings("removal")
+ @Deprecated
Review Comment:
Is there a reason why this is `@Deprecated` vs `@SuppressWarnings("deprecation")`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove and prevent use of terminal deprecations [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4032:
URL: https://github.com/apache/accumulo/pull/4032#discussion_r1418121943
##########
core/src/main/java/org/apache/accumulo/core/manager/balancer/TabletStatisticsImpl.java:
##########
@@ -46,7 +46,7 @@ public long getNumEntries() {
}
@Override
- @SuppressWarnings("removal")
+ @Deprecated
Review Comment:
Is there a reason why this is `@Deprecated` vs `@SuppressWarnings("deprecated")`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove and prevent use of terminal deprecations [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #4032:
URL: https://github.com/apache/accumulo/pull/4032
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove and prevent use of terminal deprecations [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4032:
URL: https://github.com/apache/accumulo/pull/4032#discussion_r1418129373
##########
core/src/main/java/org/apache/accumulo/core/manager/balancer/TabletStatisticsImpl.java:
##########
@@ -46,7 +46,7 @@ public long getNumEntries() {
}
@Override
- @SuppressWarnings("removal")
+ @Deprecated
Review Comment:
Chatted with @ctubbsii and responding here to resolve this comment.
So this is a method that uses an interface which is deprecated. We need to also deprecate the method since removal of the interface would still allow this method to be referenced without errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove and prevent use of terminal deprecations [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4032:
URL: https://github.com/apache/accumulo/pull/4032#discussion_r1418028087
##########
pom.xml:
##########
@@ -1042,6 +1042,10 @@
<property name="format" value="\s+$" />
<property name="message" value="Line has trailing whitespace." />
</module>
+ <module name="RegexpSinglelineJava">
+ <property name="format" value="[@]Deprecated([^)]*forRemoval[^)]*)" />
+ <property name="message" value="forRemoval should not be used." />
+ </module>
Review Comment:
This is a good addition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org