Mark TabletStatistics.getSplitCreationTime as deprecated#3977
Mark TabletStatistics.getSplitCreationTime as deprecated#3977dlmarion merged 2 commits intoapache:mainfrom
Conversation
|
#3959 removed TabletStatistics.getCreationTime in the elasticity branch. |
core/src/main/java/org/apache/accumulo/core/spi/balancer/data/TabletStatistics.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| @SuppressWarnings("removal") |
There was a problem hiding this 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
Show resolved
Hide resolved
Rewrite MemoryConsumingIterator's method to compute the amount of memory to consume, so that: * The implementation is more comprehensible * Replace exception with bounds checking * Avoid allocating more than necessary (a single byte is sufficient) * The log message includes the amount of used memory detected * The waiting message appears, even when memory was detected, because that's the behavior * Give the GC more time to detect the changed GC condition before trying to detect the low memory condition Also, remove hard-coded comments for size of heap and incorrect interval frequency, and increase the configured free memory threshold, so that the memory percentage isn't so low, it doesn't get lower than the minimum that G1GC needs to do its job by default on a 256K VM. This fixes apache#3868 Also include trivial fixes: * Fix deprecation warning issues for getSplitCreationTime by making impl class deprecated instead of suppressing the interface deprecation, and use regular deprecations, not forRemoval=true, which complicates the way deprecations get inherited (see comments on apache#3977) * Remove unused Logger
|
I suggested changes to the way it's marked as deprecated in #4024 as a trivial change in conjunction with the main point of that PR. I can separate it out if needed. |
|
I keep getting conflicting advice on whether to use the 'forRemoval' attribute. As you can see from the comments above, I didn't use it at first, then it was suggested that I add it. I do agree it seems redundant, but it must exist for some reason. Personally, I don't care whether we use it or not. Maybe we can decide at a project level? |
I'm not sure who is in favor of it, but I imagine whoever is, it's because it's helpful to be explicit. I suspect Java added it because so many people mark things deprecated and then they stayed deprecated forever in some Java libraries. But that's not really necessary in a world where semantic versioning is used and major releases are expected for breaking changes. I have personally avoided it because of the problems that it causes but I haven't really objected to using it in principal. Any objections that I might have her purely practical.
The options would be to use it everywhere, use it nowhere, not care if it's used or not, or develop some criteria by which we decide when it should be used and when it shouldn't. Since semantic versioning drives us, I don't really think that last option is useful. The third option can lead to confusion. But I think for practical reasons using it everywhere isn't really viable. So that really only leaves use it nowhere. But we can discuss and decide on the mailing list. |
There are other Deprecated annotations in Propery.java using the forRemoval annotation in main at this point. It might make sense to remove these changes from #4024 and change them all in one PR. Maybe that's the appropriate place to have that conversation. |
3 participants
No description provided.