You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/11/16 07:11:26 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request, #3080: Remove deprecated replication
ctubbsii opened a new pull request, #3080:
URL: https://github.com/apache/accumulo/pull/3080
This is a work in progress
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #3080: Remove deprecated replication
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3080:
URL: https://github.com/apache/accumulo/pull/3080#issuecomment-1316490539
This would remove a little less than 5% of our lines of code, but would still need follow on work to remove the replication table when upgrading from an earlier version.
--
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
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3080: Remove deprecated replication
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #3080:
URL: https://github.com/apache/accumulo/pull/3080#discussion_r1027338656
##########
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:
I also like removing the method since it results in a compile time error instead of a run time one as others have mentioned. Personally I like semver because when followed it usually means users have less minutiae they have to know to correctly use a project. Following semver in this case seems like it would add minutia.
--
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
[GitHub] [accumulo] ctubbsii merged pull request #3080: Remove deprecated replication
Posted by GitBox <gi...@apache.org>.
ctubbsii merged PR #3080:
URL: https://github.com/apache/accumulo/pull/3080
--
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
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3080: Remove deprecated replication
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #3080:
URL: https://github.com/apache/accumulo/pull/3080#discussion_r1026392482
##########
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:
I'm wondering if the following is sufficient (I think it is):
* Modify the 2.1.0 release notes to add a "Newly Deprecated" section and state that the entire replication feature is deprecated (in big, red, flashing font). The fact that the deprecation annotations are missing on these few methods in 2.1.0 is a bug in of itself.
* Add the missing deprecation annotations in 2.1.1
* Remove in 3.0.0
--
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
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3080: Remove deprecated replication
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3080:
URL: https://github.com/apache/accumulo/pull/3080#discussion_r1026535384
##########
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:
Okay, based on the feedback, aside from the big red flashing font (regular fonts should be fine), I think adding to the 2.1.0 release notes, adding the annotation in the 2.1.1, and removal in 3.0.0 makes the most sense.
--
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
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3080: Remove deprecated replication
Posted by 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
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3080: Remove deprecated replication
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3080:
URL: https://github.com/apache/accumulo/pull/3080#discussion_r1026577343
##########
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:
Any other substantial deprecations in 2.1 we should mention as well?
--
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
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3080: Remove deprecated replication
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #3080:
URL: https://github.com/apache/accumulo/pull/3080#discussion_r1025583498
##########
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:
If you add deprecation annotations in 2.1.1, then we can remove these, right?
--
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
[GitHub] [accumulo] cshannon commented on a diff in pull request #3080: Remove deprecated replication
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #3080:
URL: https://github.com/apache/accumulo/pull/3080#discussion_r1026319676
##########
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:
I'm definitely in favor of removal in 3.0.0 since it's a major version update and that is the time to make API changes. If we had to pick one of the other 2 then I'd prefer UnsupportedOperationException over NOOP, but removal is the best to me.
Normally when you mark something as deprecated it is still functional but just not recommended and the plan is to remove it in the future (but still currently works), so marking it in 2.1.1 as deprecated makes sense as the code will still be there.
However for version 3.0.0, if all the functionality has been removed, I see no point of leaving in the methods. As you pointed out already, UnsupportedOperationException is not great because instead of catching the error early you now delay it to runtime. NOOP to me is the worst option as that is very misleading to leave methods around that don't do anything and could cause compile time issues later if you wanted to remove it. Someone might not pay attention and miss the deprecation warnings and now when they update the methods just don't do anything with no obvious warning or error, which could be even worse than a runtime error.
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #3080: Remove deprecated replication
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3080:
URL: https://github.com/apache/accumulo/pull/3080#issuecomment-1318589468
All ITs passed with these changes
--
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
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3080: Remove deprecated replication
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #3080:
URL: https://github.com/apache/accumulo/pull/3080#discussion_r1026537151
##########
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:
lol - right, I just meant that we should make it very visible - maybe add it to the top of the release notes.
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #3080: Remove deprecated replication
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3080:
URL: https://github.com/apache/accumulo/pull/3080#issuecomment-1331485622
Found another flaky SASL/Kerberos test (ThriftTransportKeyTest) due to JVM reuse, just like in #3074 . Running it in a separate JVM seems to fix it. It's not related to these changes, but needed to isolate it to get it to reliably pass the tests in GitHub Actions. I'm still not sure the underlying actual issue with Hadoop's UserGroupInformation that is causing these, but `UserGroupInformation.initialize`doesn't seem to like being called twice in the same JVM, and assignment to one particular JVM or another by the surefire plugin seems to be flaky, based on environmental conditions.
--
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
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3080: Remove deprecated replication
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #3080:
URL: https://github.com/apache/accumulo/pull/3080#discussion_r1026588081
##########
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:
There are several others called out in the release notes, but they are buried.
VFSClassLoader
CompactionStrategy
Master to Manager seems well documented in the release notes.
--
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
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3080: Remove deprecated replication
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3080:
URL: https://github.com/apache/accumulo/pull/3080#discussion_r1027678627
##########
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:
Okay, I created #3090 for 2.1.1. I'll update this PR after I merge that in. Will still need to update the release notes for 2.1.0.
--
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