[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3080: Remove deprecated replication

[GitBox <gi...@apache.org>]

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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org