ctubbsii commented on code in PR #3977:
URL: https://github.com/apache/accumulo/pull/3977#discussion_r1416237860
##########
core/src/main/java/org/apache/accumulo/core/manager/balancer/TabletStatisticsImpl.java:
##########
@@ -46,6 +46,7 @@ public long getNumEntries() {
}
@Override
+ @SuppressWarnings("removal")
Review Comment:
Why was this warnings suppression added here? I don't think this is correct.
You don't need to suppress the warning in the subclass for the deprecation in
the interface. However, it may be necessary to mark the implementation as also
deprecated, in case it's used directly instead of only via the interface.
##########
core/src/main/java/org/apache/accumulo/core/spi/balancer/data/TabletStatistics.java:
##########
@@ -28,6 +28,7 @@ public interface TabletStatistics extends
Comparable<TabletStatistics> {
long getNumEntries();
+ @Deprecated(since = "3.1", forRemoval = true)
Review Comment:
I'm still torn on whether we should bother marking things as "forRemoval" or
not, at all. On the one hand, it's nice to be explicit, but on the other hand,
all our deprecations are always subject to removal, causing a breaking change
on a major release, in accordance with semver--so, it's kind of redundant, and
kind of feels like we're just using it because it's there, when we don't really
need to.
The other issue is this Eclipse bug that makes it hard to properly suppress
"removal". See https://bugs.eclipse.org/bugs/show_bug.cgi?id=565271
Other IDEs may also be affected by improperly handling this new annotation
attribute.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
