Delete old deprecated code #859
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Oct 10, 2017
|
retest this please |
|
Apache jenkins failed with: The last thing in the console before that was the log below (looks like it hung when trying to execute |
|
retest this please |
|
I ran this test many times ( |
tbouron
reviewed
Oct 13, 2017
| * | ||
| * @see {@link #getConfig(ConfigKey)} | ||
| */ | ||
| <T> T getConfig(HasConfigKey<T> key); |
There was a problem hiding this comment.
Ooops, went a bit too far on this one, isn't it?
There was a problem hiding this comment.
Great spot - thanks for checking thoroughly!
| @Deprecated /** @deprecated since 0.9.0; becoming private because we should now always have a registered type callers can pass instead of the catalog item id */ | ||
| public static BrooklynClassLoadingContext newClassLoadingContext(@Nullable ManagementContext mgmt, String catalogItemId, Collection<? extends OsgiBundleWithUrl> libraries, BrooklynClassLoadingContext loader) { | ||
| @Deprecated /** @deprecated since 0.9.0; we should now always have a registered type callers can pass instead of the catalog item id */ | ||
| private static BrooklynClassLoadingContext newClassLoadingContext(@Nullable ManagementContext mgmt, String catalogItemId, Collection<? extends OsgiBundleWithUrl> libraries, BrooklynClassLoadingContext loader) { |
There was a problem hiding this comment.
Keeping for persistence reason I suppose?
There was a problem hiding this comment.
The other three public methods all delegate to this; we just want this one to be private.
I expect there's scope to re-write the methods to delete this, but I've not looked closely (hence leaving it as deprecated).
| public static <T> Predicate<Entity> withLocation(final Location location) { | ||
| return locationsIncludes(location); | ||
| } | ||
|
|
||
| /** @deprecated since 0.7.0 use {@link #locationsIncludes(Location)}, introduced to allow deserialization of anonymous inner class */ | ||
| @SuppressWarnings("unused") @Deprecated | ||
| private static <T> Predicate<Entity> withLocationOld(final Location location) { |
There was a problem hiding this comment.
Can we remove this one? Don't think it is use for rebind
There was a problem hiding this comment.
If someone has really old historic persisted state then they might reference this anonymous class!
At some point (when we have a way for the user to fix their persisted state easily?!) we'll delete this stuff.
| public static <T> Predicate<Entity> managed() { | ||
| return isManaged(); | ||
| } | ||
|
|
||
| /** @deprecated since 0.7.0 use {@link #isManaged()}, introduced to allow deserialization of anonymous inner class */ | ||
| @SuppressWarnings("unused") @Deprecated | ||
| private static <T> Predicate<Entity> managedOld() { |
There was a problem hiding this comment.
Can we remove this one? Don't think it is use for rebind
There was a problem hiding this comment.
As above: playing it safe in case someone has extremely old persisted state (until we have a nicer way to walk users though detecting/fixing such problems).
| @@ -110,8 +110,6 @@ public File createJarFromClasspathDir(String path) { | |||
| addUrlItemRecursively(zout, path, path, Predicates.alwaysTrue()); | |||
| } | |||
|
|
|||
| Streams.closeQuietly(zout); | |||
There was a problem hiding this comment.
Yes - it's done in the finally block as well, so this line is redundant (admittedly not to do with deprecated code, but is general code cleanup).
aledsage
force-pushed
the
delete-old-deprecated-code
branch
from
8594e65
to
847a4da
Compare
|
Yup @aledsage, LGTM. |
|
@aledsage Build failure seems related though: |
aledsage
force-pushed
the
delete-old-deprecated-code
branch
from
847a4da
to
4f86cfd
Compare
|
retest this please |
1 similar comment
|
retest this please |
|
Test failure looked unrelated, and I couldn't reproduce locally (or see why this would fail!): |
|
retest this please |
|
Failed again, this time when running Higher in the test log, it says: |
|
retest this please |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this PR, I've only deleted things deprecated in 0.9.0 or before. I haven't deleted all of it either - I didn't delete the deprecated code that was still in use in non-trivial ways.
We could do with another look through the code for more things to delete, and could also look at deleting things deprecated since 0.10.0 (released 21 Dec 2016 - http://search.maven.org/#search%7Cgav%7C1%7Cg%3A%22org.apache.brooklyn%22%20AND%20a%3A%22brooklyn-core%22).
I've tried to be conservative, to not delete anything that might affect historic persisted state. For example, the only classes I've deleted are ones that I'd really hope are not referenced in anyone's really old persisted state!
This will require some other minor changes in brooklyn-library etc (to remove use of deprecated code), and will likely affect some downstream projects (e.g. https://github.com/brooklyncentral/advanced-networking).