You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/05/15 13:59:26 UTC
[GitHub] [hadoop-ozone] mukul1987 opened a new pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
mukul1987 opened a new pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924
## What changes were proposed in this pull request?
This patch adds a protobuf backward compatibility checker into Ozone.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3595
## How was this patch tested?
Build works, and able to detect failure because of this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx commented on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-629529457
> I found the plugin to be useful to detect backward compatibility issues. However I still am not completely sure about LICENSE and checkstyle and other related issue.
This will be very useful @mukul1987. The license is '**BSD 3-Clause "New" or "Revised" License**'
_- Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.
- Neither the name of Salesforce.com nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission._
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-630721102
One note: LICENSE.txt shouldn't be modified. We don't include neither the code nor the binary of the plugin in any of the releases. Will revert that line during the commit if no objection.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#discussion_r426490028
##########
File path: hadoop-hdds/common/pom.xml
##########
@@ -202,6 +202,20 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
</extension>
</extensions>
<plugins>
+ <plugin>
+ <groupId>com.salesforce.servicelibs</groupId>
+ <artifactId>proto-backwards-compatibility</artifactId>
+ <configuration>
+ <protoSourceRoot>${basedir}/src/main/proto/</protoSourceRoot>
+ </configuration>
+ <executions>
+ <execution>
+ <goals>
+ <goal>backwards-compatibility-check</goal>
+ </goals>
+ </execution>
+ </executions>
Review comment:
Can we avoid multiplication of this config by adding it to the root pom instead?
##########
File path: hadoop-hdds/container-service/pom.xml
##########
@@ -100,6 +100,20 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
<build>
<plugins>
+ <plugin>
+ <groupId>com.salesforce.servicelibs</groupId>
+ <artifactId>proto-backwards-compatibility</artifactId>
Review comment:
The plugin's doc says [`os-maven-plugin` is required for it to work](https://github.com/salesforce/proto-backwards-compat-maven-plugin#usage), but it's missing from `hadoop-hdds/container-service/pom.xml` and `hadoop-ozone/common/pom.xml` (only present in the other two submodule pom files).
##########
File path: pom.xml
##########
@@ -241,6 +242,11 @@ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xs
<dependencyManagement>
<dependencies>
+ <dependency>
+ <groupId>com.salesforce.servicelibs</groupId>
+ <artifactId>proto-backwards-compatibility</artifactId>
+ <version>${proto-backwards-compatibility.version}</version>
Review comment:
I think this should be added to/as `<pluginManagement>/<plugin>` instead of `<dependencyManagement>/<dependency>`. Currently Maven gives warning due to missing version:
```
[WARNING] Some problems were encountered while building the effective model for org.apache.hadoop:hadoop-hdds-common:jar:0.6.0-SNAPSHOT
[WARNING] 'build.plugins.plugin.version' for com.salesforce.servicelibs:proto-backwards-compatibility is missing. @ line 205, column 15
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] mukul1987 commented on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-630295146
@adoroszlai , Thanks for the review. Have addressed the comments in the followup patch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-630714882
Thanks the answers @mukul1987
> backward compatibility should be on both the protos both for express as well as rolling upgrades.
As far as I know there is no plan to support rolling upgrade, we don't need to force backward compatibility for RPC.
>> What should we put to the lock file? Is the lock file from the last release should be added and upgrade with each release? Or the first/(last?) version from the previous major line?
> The lock file should be committed along with the source code to identify backward incompatibility issues.
I understand this part, The question was about the details. Which version is committed and when?
During an offline discussion we agreed that we can keep the version from the previous release until the next incompatible change. In case of incompatible change (right now) we should update the lock file *and* explain in the PR why did we update it. By default we should keep the lock file untouched (IMHO).
We can also make the process of updating the lock file during the release process more automatic.
But these are just random thoughts. Let's merge this PR first, start to use it, and we can adjust (and document !!) our usage patterns based on the experiences...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek closed pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
elek closed pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-630723124
Merged. Thanks @mukul1987 the patch and @adoroszlai the review and suggestions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek edited a comment on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
elek edited a comment on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-630008829
Thanks to open this issue @mukul1987
I like this approach but I have a few practical questions:
* Do we need to check the backward compatibility for RPC or just for the persisted data? If only for the persisted data, it would be more useful to separated the two RPC set.
* What is the suggested way to use this? I assume that we can do any incompatible change with upgrading the lock file together with the proto.
* What should we put to the lock file? Is the lock file from the last release should be added and upgrade with each release? Or the first/(last?) version from the previous major line?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] mukul1987 commented on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-630301975
@elek, can you please have another look at this change ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] maobaolong commented on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
maobaolong commented on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-629989339
@mukul1987 This feature looks good, very useful for keep `backward compatibility`. 👍
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#discussion_r426764724
##########
File path: pom.xml
##########
@@ -1596,8 +1602,29 @@ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xs
</dependencyManagement>
<build>
+ <extensions>
+ <extension>
+ <groupId>kr.motd.maven</groupId>
+ <artifactId>os-maven-plugin</artifactId>
+ <version>${os-maven-plugin.version}</version>
+ </extension>
+ </extensions>
<pluginManagement>
<plugins>
+ <plugin>
+ <groupId>com.salesforce.servicelibs</groupId>
+ <artifactId>proto-backwards-compatibility</artifactId>
Review comment:
```suggestion
<groupId>com.salesforce.servicelibs</groupId>
<artifactId>proto-backwards-compatibility</artifactId>
<version>${proto-backwards-compatibility.version}</version>
```
##########
File path: pom.xml
##########
@@ -241,6 +242,11 @@ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xs
<dependencyManagement>
<dependencies>
+ <dependency>
+ <groupId>com.salesforce.servicelibs</groupId>
+ <artifactId>proto-backwards-compatibility</artifactId>
+ <version>${proto-backwards-compatibility.version}</version>
+ </dependency>
Review comment:
```suggestion
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx edited a comment on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
avijayanhwx edited a comment on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-629529457
> I found the plugin to be useful to detect backward compatibility issues. However I still am not completely sure about LICENSE and checkstyle and other related issue.
This will be very useful @mukul1987. The license is '**BSD 3-Clause "New" or "Revised" License**'
_Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.
Neither the name of Salesforce.com nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission._
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx edited a comment on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
avijayanhwx edited a comment on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-629529457
> I found the plugin to be useful to detect backward compatibility issues. However I still am not completely sure about LICENSE and checkstyle and other related issue.
This will be very useful @mukul1987. The patch looks straightforward to me. The license is '**BSD 3-Clause "New" or "Revised" License**'
_Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.
Neither the name of Salesforce.com nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission._
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] mukul1987 edited a comment on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
mukul1987 edited a comment on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-630301738
> Do we need to check the backward compatibility for RPC or just for the persisted data? If only for the persisted data, it would be more useful to separated the two RPC set.
backward compatibility should be on both the protos both for express as well as rolling upgrades.
> What is the suggested way to use this? I assume that we can do any incompatible change with upgrading the lock file together with the proto.
this fails the compilation with an error so that the user can look into the error and identify that the change is backward incompatible.
> What should we put to the lock file? Is the lock file from the last release should be added and upgrade with each release? Or the first/(last?) version from the previous major line?
The lock file should be committed along with the source code to identify backward incompatibility issues.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] mukul1987 commented on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-629252681
I found the plugin to be useful to detect backward compatibility issues. However I still am not completely sure about LICENSE and checkstyle and other related issue.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] mukul1987 commented on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-630301738
Do we need to check the backward compatibility for RPC or just for the persisted data? If only for the persisted data, it would be more useful to separated the two RPC set.
>> backward compatibility should be on both the protos both for express as well as rolling upgrades.
What is the suggested way to use this? I assume that we can do any incompatible change with upgrading the lock file together with the proto.
>> this fails the compilation with an error so that the user can look into the error and identify that the change is backward incompatible.
What should we put to the lock file? Is the lock file from the last release should be added and upgrade with each release? Or the first/(last?) version from the previous major line?
>> The lock file should be committed along with the source code to identify backward incompatibility issues.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #924: HDDS-3595. Add a maven proto file backward compatibility checker in Ozone.
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #924:
URL: https://github.com/apache/hadoop-ozone/pull/924#issuecomment-630008829
Thanks to open this issue @mukul1987
I have a few questions:
* Do we need to check the backward compatibility for RPC or just for the persisted data? If only for the persisted data, it would be more useful to separated the two RPC set.
* What is the suggested way to use this? I assume that we can do any incompatible change with upgrading the lock file together with the proto.
* What should we put to the lock file? Is the lock file from the last release should be added and upgrade with each release? Or the first/(last?) version from the previous major line?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org