stoty commented on code in PR #147:
URL: https://github.com/apache/phoenix-omid/pull/147#discussion_r1371360421
##########
pom.xml:
##########
@@ -367,54 +367,58 @@
<version>${maven-license-plugin.version}</version>
<inherited>true</inherited> <!-- for multi-module config -->
<configuration>
- <header>${license.header}</header>
<aggregate>true</aggregate> <!-- for multi-module config
-->
<skipExistingHeaders>false</skipExistingHeaders>
- <includes>
- <!-- Add files where to include licensing -->
- <include>src/**/*.java</include>
- <include>src/**/*.xml</include>
- <include>src/**/*.proto</include>
- <include>**/*.css</include>
- <include>**/*.sh</include>
- </includes>
- <excludes>
- <!-- Add files to exclude from licensing -->
+ <!-- Add comment style for protocol buffers -->
+ <mapping>
+ <java>SLASHSTAR_STYLE</java>
+ <proto>DOUBLESLASH_STYLE</proto>
Review Comment:
Just replace the ASL Header in the proto file with the standard Slashstar
format one ?
That's what HBase and Phoenix uses.
##########
pom.xml:
##########
@@ -367,54 +367,58 @@
<version>${maven-license-plugin.version}</version>
<inherited>true</inherited> <!-- for multi-module config -->
<configuration>
- <header>${license.header}</header>
<aggregate>true</aggregate> <!-- for multi-module config
-->
<skipExistingHeaders>false</skipExistingHeaders>
- <includes>
- <!-- Add files where to include licensing -->
- <include>src/**/*.java</include>
- <include>src/**/*.xml</include>
- <include>src/**/*.proto</include>
- <include>**/*.css</include>
- <include>**/*.sh</include>
- </includes>
- <excludes>
- <!-- Add files to exclude from licensing -->
+ <!-- Add comment style for protocol buffers -->
+ <mapping>
+ <java>SLASHSTAR_STYLE</java>
+ <proto>DOUBLESLASH_STYLE</proto>
+ </mapping>
+ <licenseSets>
+ <licenseSet>
+ <header>${LicenseSet.header}</header>
+ <includes>
+ <!-- Add files where to include licensing -->
+ <include>src/**/*.java</include>
+ <include>src/**/*.xml</include>
+ <include>src/**/*.proto</include>
+ <include>**/*.css</include>
+ <include>**/*.sh</include>
+ </includes>
+ <excludes>
+ <!-- Add files to exclude from licensing -->
- <!-- Exclude pom -->
- <exclude>**/pom.xml</exclude>
+ <!-- Exclude pom -->
+ <exclude>**/pom.xml</exclude>
- <!-- Exclude config and scripts -->
- <exclude>**/dev-utils/*</exclude>
- <exclude>dev-support/artifacts/**</exclude>
- <exclude>dev-support/work/**</exclude>
+ <!-- Exclude config and scripts -->
+ <exclude>**/dev-utils/*</exclude>
+ <exclude>dev-support/artifacts/**</exclude>
+ <exclude>dev-support/work/**</exclude>
- <!-- Exclude assembly -->
- <exclude>**/maven/assembly/*</exclude>
+ <!-- Exclude assembly -->
+ <exclude>**/maven/assembly/*</exclude>
- <!-- Project specific exclusions (e.g. they've been
copied/modified from other open-source projects -->
+ <!-- Project specific exclusions (e.g. they've
been copied/modified from other open-source projects -->
- <!-- Protocol buffers generated classes -->
-
<exclude>**/src/main/java/org/apache/omid/proto/TSOProto.java</exclude>
+ <!-- Protocol buffers generated classes -->
+
<exclude>**/src/main/java/org/apache/omid/proto/TSOProto.java</exclude>
- <!-- Taken from
https://github.com/brianfrankcooper/YCSB -->
-
<exclude>**/src/main/java/org/apache/omid/benchmarks/utils/Generator.java</exclude>
-
<exclude>**/src/main/java/org/apache/omid/benchmarks/utils/IntegerGenerator.java</exclude>
-
<exclude>**/src/main/java/org/apache/omid/benchmarks/utils/ZipfianGenerator.java</exclude>
-
<exclude>**/src/main/java/org/apache/omid/benchmarks/utils/ScrambledZipfianGenerator.java
- </exclude>
+ <!-- Taken from
https://github.com/brianfrankcooper/YCSB -->
+
<exclude>**/src/main/java/org/apache/omid/benchmarks/utils/Generator.java</exclude>
Review Comment:
These should not be excluded.
Instead, the if the license is indeed ASL compatible (I think it is), then
it should be added as a valid license.
##########
pom.xml:
##########
@@ -367,54 +367,58 @@
<version>${maven-license-plugin.version}</version>
<inherited>true</inherited> <!-- for multi-module config -->
<configuration>
- <header>${license.header}</header>
<aggregate>true</aggregate> <!-- for multi-module config
-->
<skipExistingHeaders>false</skipExistingHeaders>
- <includes>
- <!-- Add files where to include licensing -->
- <include>src/**/*.java</include>
- <include>src/**/*.xml</include>
- <include>src/**/*.proto</include>
- <include>**/*.css</include>
- <include>**/*.sh</include>
- </includes>
- <excludes>
- <!-- Add files to exclude from licensing -->
+ <!-- Add comment style for protocol buffers -->
+ <mapping>
+ <java>SLASHSTAR_STYLE</java>
+ <proto>DOUBLESLASH_STYLE</proto>
+ </mapping>
+ <licenseSets>
+ <licenseSet>
+ <header>${LicenseSet.header}</header>
+ <includes>
Review Comment:
Maybe add Yaml files, and add ASL header there ?
##########
pom.xml:
##########
@@ -367,54 +367,58 @@
<version>${maven-license-plugin.version}</version>
<inherited>true</inherited> <!-- for multi-module config -->
<configuration>
- <header>${license.header}</header>
<aggregate>true</aggregate> <!-- for multi-module config
-->
<skipExistingHeaders>false</skipExistingHeaders>
- <includes>
- <!-- Add files where to include licensing -->
- <include>src/**/*.java</include>
- <include>src/**/*.xml</include>
- <include>src/**/*.proto</include>
- <include>**/*.css</include>
- <include>**/*.sh</include>
- </includes>
- <excludes>
- <!-- Add files to exclude from licensing -->
+ <!-- Add comment style for protocol buffers -->
+ <mapping>
+ <java>SLASHSTAR_STYLE</java>
+ <proto>DOUBLESLASH_STYLE</proto>
+ </mapping>
+ <licenseSets>
+ <licenseSet>
+ <header>${LicenseSet.header}</header>
+ <includes>
+ <!-- Add files where to include licensing -->
+ <include>src/**/*.java</include>
+ <include>src/**/*.xml</include>
+ <include>src/**/*.proto</include>
+ <include>**/*.css</include>
+ <include>**/*.sh</include>
+ </includes>
+ <excludes>
+ <!-- Add files to exclude from licensing -->
- <!-- Exclude pom -->
- <exclude>**/pom.xml</exclude>
+ <!-- Exclude pom -->
+ <exclude>**/pom.xml</exclude>
- <!-- Exclude config and scripts -->
- <exclude>**/dev-utils/*</exclude>
- <exclude>dev-support/artifacts/**</exclude>
- <exclude>dev-support/work/**</exclude>
+ <!-- Exclude config and scripts -->
+ <exclude>**/dev-utils/*</exclude>
+ <exclude>dev-support/artifacts/**</exclude>
Review Comment:
Can you check all of these ?
Looks like this was copied from HBase or Phoenix.
We should clean up the non-existent directories and files
(except for the ones generated during build)
##########
pom.xml:
##########
@@ -367,54 +367,58 @@
<version>${maven-license-plugin.version}</version>
<inherited>true</inherited> <!-- for multi-module config -->
<configuration>
- <header>${license.header}</header>
<aggregate>true</aggregate> <!-- for multi-module config
-->
<skipExistingHeaders>false</skipExistingHeaders>
- <includes>
- <!-- Add files where to include licensing -->
- <include>src/**/*.java</include>
- <include>src/**/*.xml</include>
- <include>src/**/*.proto</include>
- <include>**/*.css</include>
- <include>**/*.sh</include>
- </includes>
- <excludes>
- <!-- Add files to exclude from licensing -->
+ <!-- Add comment style for protocol buffers -->
+ <mapping>
+ <java>SLASHSTAR_STYLE</java>
+ <proto>DOUBLESLASH_STYLE</proto>
+ </mapping>
+ <licenseSets>
+ <licenseSet>
+ <header>${LicenseSet.header}</header>
+ <includes>
+ <!-- Add files where to include licensing -->
+ <include>src/**/*.java</include>
+ <include>src/**/*.xml</include>
+ <include>src/**/*.proto</include>
+ <include>**/*.css</include>
+ <include>**/*.sh</include>
+ </includes>
+ <excludes>
+ <!-- Add files to exclude from licensing -->
- <!-- Exclude pom -->
- <exclude>**/pom.xml</exclude>
+ <!-- Exclude pom -->
+ <exclude>**/pom.xml</exclude>
- <!-- Exclude config and scripts -->
- <exclude>**/dev-utils/*</exclude>
- <exclude>dev-support/artifacts/**</exclude>
- <exclude>dev-support/work/**</exclude>
+ <!-- Exclude config and scripts -->
+ <exclude>**/dev-utils/*</exclude>
Review Comment:
There is no such file in the repo.
Can we remove this ?
##########
pom.xml:
##########
@@ -367,54 +367,58 @@
<version>${maven-license-plugin.version}</version>
<inherited>true</inherited> <!-- for multi-module config -->
<configuration>
- <header>${license.header}</header>
<aggregate>true</aggregate> <!-- for multi-module config
-->
<skipExistingHeaders>false</skipExistingHeaders>
- <includes>
- <!-- Add files where to include licensing -->
- <include>src/**/*.java</include>
- <include>src/**/*.xml</include>
- <include>src/**/*.proto</include>
- <include>**/*.css</include>
- <include>**/*.sh</include>
- </includes>
- <excludes>
- <!-- Add files to exclude from licensing -->
+ <!-- Add comment style for protocol buffers -->
+ <mapping>
+ <java>SLASHSTAR_STYLE</java>
+ <proto>DOUBLESLASH_STYLE</proto>
+ </mapping>
+ <licenseSets>
+ <licenseSet>
+ <header>${LicenseSet.header}</header>
+ <includes>
+ <!-- Add files where to include licensing -->
+ <include>src/**/*.java</include>
+ <include>src/**/*.xml</include>
+ <include>src/**/*.proto</include>
+ <include>**/*.css</include>
+ <include>**/*.sh</include>
+ </includes>
+ <excludes>
+ <!-- Add files to exclude from licensing -->
- <!-- Exclude pom -->
- <exclude>**/pom.xml</exclude>
+ <!-- Exclude pom -->
+ <exclude>**/pom.xml</exclude>
- <!-- Exclude config and scripts -->
- <exclude>**/dev-utils/*</exclude>
- <exclude>dev-support/artifacts/**</exclude>
- <exclude>dev-support/work/**</exclude>
+ <!-- Exclude config and scripts -->
+ <exclude>**/dev-utils/*</exclude>
+ <exclude>dev-support/artifacts/**</exclude>
+ <exclude>dev-support/work/**</exclude>
- <!-- Exclude assembly -->
- <exclude>**/maven/assembly/*</exclude>
+ <!-- Exclude assembly -->
+ <exclude>**/maven/assembly/*</exclude>
- <!-- Project specific exclusions (e.g. they've been
copied/modified from other open-source projects -->
+ <!-- Project specific exclusions (e.g. they've
been copied/modified from other open-source projects -->
- <!-- Protocol buffers generated classes -->
-
<exclude>**/src/main/java/org/apache/omid/proto/TSOProto.java</exclude>
+ <!-- Protocol buffers generated classes -->
+
<exclude>**/src/main/java/org/apache/omid/proto/TSOProto.java</exclude>
Review Comment:
No such file, we are generating this dynamically now.
##########
pom.xml:
##########
@@ -367,54 +367,58 @@
<version>${maven-license-plugin.version}</version>
<inherited>true</inherited> <!-- for multi-module config -->
<configuration>
- <header>${license.header}</header>
<aggregate>true</aggregate> <!-- for multi-module config
-->
<skipExistingHeaders>false</skipExistingHeaders>
- <includes>
- <!-- Add files where to include licensing -->
- <include>src/**/*.java</include>
- <include>src/**/*.xml</include>
- <include>src/**/*.proto</include>
- <include>**/*.css</include>
- <include>**/*.sh</include>
- </includes>
- <excludes>
- <!-- Add files to exclude from licensing -->
+ <!-- Add comment style for protocol buffers -->
+ <mapping>
+ <java>SLASHSTAR_STYLE</java>
+ <proto>DOUBLESLASH_STYLE</proto>
+ </mapping>
+ <licenseSets>
+ <licenseSet>
+ <header>${LicenseSet.header}</header>
+ <includes>
+ <!-- Add files where to include licensing -->
+ <include>src/**/*.java</include>
+ <include>src/**/*.xml</include>
+ <include>src/**/*.proto</include>
+ <include>**/*.css</include>
+ <include>**/*.sh</include>
+ </includes>
+ <excludes>
+ <!-- Add files to exclude from licensing -->
- <!-- Exclude pom -->
- <exclude>**/pom.xml</exclude>
+ <!-- Exclude pom -->
+ <exclude>**/pom.xml</exclude>
Review Comment:
POM files should have an ASL header,
##########
pom.xml:
##########
@@ -367,54 +367,58 @@
<version>${maven-license-plugin.version}</version>
<inherited>true</inherited> <!-- for multi-module config -->
<configuration>
- <header>${license.header}</header>
<aggregate>true</aggregate> <!-- for multi-module config
-->
<skipExistingHeaders>false</skipExistingHeaders>
- <includes>
- <!-- Add files where to include licensing -->
- <include>src/**/*.java</include>
- <include>src/**/*.xml</include>
- <include>src/**/*.proto</include>
- <include>**/*.css</include>
- <include>**/*.sh</include>
- </includes>
- <excludes>
- <!-- Add files to exclude from licensing -->
+ <!-- Add comment style for protocol buffers -->
+ <mapping>
+ <java>SLASHSTAR_STYLE</java>
+ <proto>DOUBLESLASH_STYLE</proto>
+ </mapping>
+ <licenseSets>
+ <licenseSet>
+ <header>${LicenseSet.header}</header>
+ <includes>
+ <!-- Add files where to include licensing -->
+ <include>src/**/*.java</include>
+ <include>src/**/*.xml</include>
+ <include>src/**/*.proto</include>
+ <include>**/*.css</include>
+ <include>**/*.sh</include>
+ </includes>
+ <excludes>
+ <!-- Add files to exclude from licensing -->
- <!-- Exclude pom -->
- <exclude>**/pom.xml</exclude>
+ <!-- Exclude pom -->
+ <exclude>**/pom.xml</exclude>
- <!-- Exclude config and scripts -->
- <exclude>**/dev-utils/*</exclude>
- <exclude>dev-support/artifacts/**</exclude>
- <exclude>dev-support/work/**</exclude>
+ <!-- Exclude config and scripts -->
+ <exclude>**/dev-utils/*</exclude>
+ <exclude>dev-support/artifacts/**</exclude>
+ <exclude>dev-support/work/**</exclude>
- <!-- Exclude assembly -->
- <exclude>**/maven/assembly/*</exclude>
+ <!-- Exclude assembly -->
+ <exclude>**/maven/assembly/*</exclude>
- <!-- Project specific exclusions (e.g. they've been
copied/modified from other open-source projects -->
+ <!-- Project specific exclusions (e.g. they've
been copied/modified from other open-source projects -->
- <!-- Protocol buffers generated classes -->
-
<exclude>**/src/main/java/org/apache/omid/proto/TSOProto.java</exclude>
+ <!-- Protocol buffers generated classes -->
+
<exclude>**/src/main/java/org/apache/omid/proto/TSOProto.java</exclude>
- <!-- Taken from
https://github.com/brianfrankcooper/YCSB -->
-
<exclude>**/src/main/java/org/apache/omid/benchmarks/utils/Generator.java</exclude>
-
<exclude>**/src/main/java/org/apache/omid/benchmarks/utils/IntegerGenerator.java</exclude>
-
<exclude>**/src/main/java/org/apache/omid/benchmarks/utils/ZipfianGenerator.java</exclude>
-
<exclude>**/src/main/java/org/apache/omid/benchmarks/utils/ScrambledZipfianGenerator.java
- </exclude>
+ <!-- Taken from
https://github.com/brianfrankcooper/YCSB -->
+
<exclude>**/src/main/java/org/apache/omid/benchmarks/utils/Generator.java</exclude>
+
<exclude>**/src/main/java/org/apache/omid/benchmarks/utils/IntegerGenerator.java</exclude>
+
<exclude>**/src/main/java/org/apache/omid/benchmarks/utils/ZipfianGenerator.java</exclude>
+
<exclude>**/src/main/java/org/apache/omid/benchmarks/utils/ScrambledZipfianGenerator.java
+ </exclude>
- <!-- Taken from https://github.com/apache/hbase -->
-
<exclude>**/src/main/java/org/apache/omid/committable/hbase/RegionSplitter.java</exclude>
+ <!-- Taken from
https://github.com/apache/hbase -->
+
<exclude>**/src/main/java/org/apache/omid/committable/hbase/RegionSplitter.java</exclude>
Review Comment:
This is already ASL.
I see no need to exclude it, just because it was copied.
--
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]
