ctubbsii commented on code in PR #3080:
URL: https://github.com/apache/accumulo/pull/3080#discussion_r1026044292
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -1501,42 +1491,36 @@ public int size() {
}
/**
- * Add a new element to the set of peers which this Mutation originated from
+ * Non-functional API; do not use
*
- * @param peer
- * the peer to add
* @since 1.7.0
+ * @deprecated old, no longer functional API for replication feature removed
in 3.0
*/
+ @Deprecated(since = "3.0.0")
Review Comment:
Semver guidelines say it should be deprecated in at least one previous minor
release before removal in a major. A patch release isn't generally sufficient.
However, there are two points that make your suggestion easier to swallow
and diverge from semver:
1. The upgrade paths we're supporting / testing are assuming you're already
on the latest patch release for the series you're on, so we can assume people
would go through at least 2.1.1 first before moving to the next major release,
giving them an opportunity to observe the deprecation warning, and
2. All the related code is already deprecated. These APIs have no value by
themselves.
If we did leave it in to be strict about semver, we'd still have the choice
of whether to make the method a NOOP or to throw an
UnsupportedOperationException. If we do the latter, then we're not really doing
user's any favors by leaving the method there... we've just made it a runtime
error instead of a more obvious compile time one.
Regardless, you're right that we should definitely mark these as deprecated
in 2.1.1. After that, we need to choose:
1. Remove
2. Keep as NOOP
3. Keep as UnsupportedOperationException
What do you think?
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -1501,42 +1491,36 @@ public int size() {
}
/**
- * Add a new element to the set of peers which this Mutation originated from
+ * Non-functional API; do not use
*
- * @param peer
- * the peer to add
* @since 1.7.0
+ * @deprecated old, no longer functional API for replication feature removed
in 3.0
*/
+ @Deprecated(since = "3.0.0")
Review Comment:
Semver guidelines say it should be deprecated in at least one previous minor
release before removal in a major. A patch release isn't generally sufficient.
However, there are two points that make your suggestion easier to swallow
and diverge from semver:
1. The upgrade paths we're supporting / testing are assuming you're already
on the latest patch release for the series you're on, so we can assume people
would go through at least 2.1.1 first before moving to the next major release,
giving them an opportunity to observe the deprecation warning, and
2. All the related code is already deprecated. These APIs have no value by
themselves.
If we did leave it in to be strict about semver, we'd still have the choice
of whether to make the method a NOOP or to throw an
UnsupportedOperationException. If we do the latter, then we're not really doing
user's any favors by leaving the method there... we've just made it a runtime
error instead of a more obvious compile time one.
Regardless, you're right that we should definitely mark these as deprecated
in 2.1.1. After that, we need to choose:
1. Remove
2. Keep as NOOP
3. Keep as UnsupportedOperationException
Which do you think?
--
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]
