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/21 07:57:07 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request, #3090: Add missing deprecation annotations in Mutation
ctubbsii opened a new pull request, #3090:
URL: https://github.com/apache/accumulo/pull/3090
Add deprecation annotations for replication APIs in Mutation that should have been included in 2.1.0 along with the rest of the replication feature in #2335, but were unintentionally overlooked at the time.
--
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 #3090: Add missing deprecation annotations in Mutation
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #3090:
URL: https://github.com/apache/accumulo/pull/3090#discussion_r1027906375
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -1507,6 +1507,7 @@ public int size() {
* the peer to add
* @since 1.7.0
*/
+ @Deprecated(since = "2.1.1")
Review Comment:
Just wondering if these should be "2.1.0". The fact that they were missing is the bug.
--
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 #3090: Add missing deprecation annotations in Mutation
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3090:
URL: https://github.com/apache/accumulo/pull/3090#discussion_r1035269006
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -1507,6 +1507,7 @@ public int size() {
* the peer to add
* @since 1.7.0
*/
+ @Deprecated(since = "2.1.1")
Review Comment:
I added a javadoc, but diverged from this suggested wording, in order to emphasize the purpose of the deprecation in 2.1.1, which is more relevant to the javadoc reader/user, rather than highlight the divergence from our normal rules, which is more relevant to us in making the decision.
--
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 #3090: Add missing deprecation annotations in Mutation
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #3090:
URL: https://github.com/apache/accumulo/pull/3090#discussion_r1029130817
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -1507,6 +1507,7 @@ public int size() {
* the peer to add
* @since 1.7.0
*/
+ @Deprecated(since = "2.1.1")
Review Comment:
Explaining what is going on in the javadoc could be in addition to updating 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] keith-turner commented on a diff in pull request #3090: Add missing deprecation annotations in Mutation
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #3090:
URL: https://github.com/apache/accumulo/pull/3090#discussion_r1029101237
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -1507,6 +1507,7 @@ public int size() {
* the peer to add
* @since 1.7.0
*/
+ @Deprecated(since = "2.1.1")
Review Comment:
Like you said @ctubbsii its about communication, so would the following work? Is it ok to use the deprecation javadoc tag and the annotation? If not could just put the explanation in the javadoc w/o the deprecation tag.
```java
* @deprecated since 2.1.1 This should have been deprecated in 2.1.0 in order to strictly follow semver. However it was missed and it must be removed in 3.0.0 if all of replication is removed. Therefore the deprecation was added in 2.1.1 which is a bit out of line with semver.
*/
@Deprecated(since = "2.1.1")
```
--
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 #3090: Add missing deprecation annotations in Mutation
Posted by GitBox <gi...@apache.org>.
ctubbsii merged PR #3090:
URL: https://github.com/apache/accumulo/pull/3090
--
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 #3090: Add missing deprecation annotations in Mutation
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3090:
URL: https://github.com/apache/accumulo/pull/3090#discussion_r1028829671
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -1507,6 +1507,7 @@ public int size() {
* the peer to add
* @since 1.7.0
*/
+ @Deprecated(since = "2.1.1")
Review Comment:
Maybe. That seems a bit misleading to me. The two perspectives are:
1. that it was deprecated in 2.1.0 but failed to be marked as such, or
2. the act of marking it deprecated is what makes it so, because deprecation is about communication, so if we didn't communicate it, it wasn't deprecated.
But then, for the second perspective, if we document on the release notes for 2.1.0 that is deprecated and should have been marked as such, then we will have communicated it.
So, I'm torn, and can see it both ways. I'd like more opinions to help swing one way or the other.
--
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 #3090: Add missing deprecation annotations in Mutation
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3090:
URL: https://github.com/apache/accumulo/pull/3090#discussion_r1030011372
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -1507,6 +1507,7 @@ public int size() {
* the peer to add
* @since 1.7.0
*/
+ @Deprecated(since = "2.1.1")
Review Comment:
@keith-turner suggested
> ```java
> * @deprecated since 2.1.1 This should have been deprecated in 2.1.0 in order to strictly follow semver. However it was missed and it must be removed in 3.0.0 if all of replication is removed. Therefore the deprecation was added in 2.1.1 which is a bit out of line with semver.
> */
> @Deprecated(since = "2.1.1")
> ```
Yes, we can add that in the javadoc, as well as the release notes. I'll take a look at how the annotation 'since' renders with the javadoc description to see if I can make them mesh well together without being redundant.
--
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 #3090: Add missing deprecation annotations in Mutation
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #3090:
URL: https://github.com/apache/accumulo/pull/3090#discussion_r1035765419
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -1507,6 +1507,7 @@ public int size() {
* the peer to add
* @since 1.7.0
*/
+ @Deprecated(since = "2.1.1")
Review Comment:
I like the final javadoc, reads nicely and communicates clearly.
--
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 #3090: Add missing deprecation annotations in Mutation
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #3090:
URL: https://github.com/apache/accumulo/pull/3090#discussion_r1029101237
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -1507,6 +1507,7 @@ public int size() {
* the peer to add
* @since 1.7.0
*/
+ @Deprecated(since = "2.1.1")
Review Comment:
Like you said @ctubbsii its about communication, so would the following work? Is it ok to use the deprecation javadoc tag and the annotation? If not could just put the explanation in the javadoc w/o the deprecation tag.
```java
* @deprecated since 2.1.1 This should have been deprecated in 2.1.0 in order to strictly follow semver. However it was missed and it must be removed in 3.0.0 if all of replication is removed.
*/
@Deprecated(since = "2.1.1")
```
--
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 #3090: Add missing deprecation annotations in Mutation
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #3090:
URL: https://github.com/apache/accumulo/pull/3090#discussion_r1029264662
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -1507,6 +1507,7 @@ public int size() {
* the peer to add
* @since 1.7.0
*/
+ @Deprecated(since = "2.1.1")
Review Comment:
> I'd like more opinions
[SemVer](https://semver.org/spec/v2.0.0#spec-item-7) says that the minor version "MUST be incremented if any public API functionality is marked as deprecated." The SemVer [FAQ](https://semver.org/spec/v2.0.0#how-should-i-handle-deprecating-functionality) about deprecation says that you should do two things when you deprecate part of the public API, "(1) update your documentation to let users know about the change, (2) issue a new minor release with the deprecation in place."
Based on an earlier conversation I believe we are planning on adding a "Deprecated Items" (or similar) section to the top of the 2.1.0 release notes. We could also state in that section that we missed a few methods in the public API and they will be fixed in a patch release. #2335 already deprecated other user facing replication related code (ReplicationOperations, Property.java). As users compile and test against 2.1.0 there will be visible signs that replication related items are deprecated via deprecation annotation warnings for ReplicationOperations and printed warnings in the server logs for the replication properties. With the changes in this paragraph, I believe that we have satisfied 1 and 2 from the FAQ.
The SemVer FAQ also contains [this](https://semver.org/spec/v2.0.0#what-if-i-inadvertently-alter-the-public-api-in-a-way-that-is-not-compliant-with-the-version-number-change-ie-the-code-incorrectly-introduces-a-major-breaking-change-in-a-patch-release). Users of the replication feature should be well aware that the feature is deprecated without having these deprecation annotations. I think it's fine to put it in the patch release.
--
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 #3090: Add missing deprecation annotations in Mutation
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3090:
URL: https://github.com/apache/accumulo/pull/3090#discussion_r1028829671
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -1507,6 +1507,7 @@ public int size() {
* the peer to add
* @since 1.7.0
*/
+ @Deprecated(since = "2.1.1")
Review Comment:
Maybe. That seems a bit misleading to me. The two perspectives are:
1. that it was deprecated in 2.1.0 but failed to be marked as such, or
2. the act of marking it deprecated is what makes it so, because deprecation is about communication, so if we didn't communicate it, it wasn't deprecated.
But then, addressing the second perspective and favoring the first, if we document on the release notes for 2.1.0 that is deprecated and should have been marked as such, then we will have communicated it.
So, I'm torn, and can see it both ways. I'd like more opinions to help swing one way or the other.
--
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