You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "chrisdutz (via GitHub)" <gi...@apache.org> on 2023/09/15 07:37:57 UTC
[GitHub] [iotdb] chrisdutz opened a new pull request, #11150: Refactor/managing dependencies
chrisdutz opened a new pull request, #11150:
URL: https://github.com/apache/iotdb/pull/11150
Enabled the enforcer plugin as well as the dependency plugin to make sure every used dependency is declared and we're not using transitive dependencies directly.
Started replacing javax packages by their jakarta counterparts
Strarted streamlining the dependencies, so we're no longer pulling in 5 different versions of the same dependency.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#issuecomment-1764231858
The rules are part of the enforcer plugin executions that are bound to the "validation" phase, which comes after "package". So a "package" build will not be affected ... I also actually didn't even add the rules, they were already in there I simply switched them to fail the build if the rules are not met. I was seeing the warnings while building all the time ... so now there should no longer be any warnings from them because I addressed what they were complaining about.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#discussion_r1359897113
##########
distribution/pom.xml:
##########
@@ -27,7 +27,43 @@
<version>1.3.0-SNAPSHOT</version>
</parent>
<artifactId>iotdb-distribution</artifactId>
+ <packaging>pom</packaging>
<name>IoTDB: Distribution</name>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.iotdb</groupId>
+ <artifactId>iotdb-server</artifactId>
+ <version>1.3.0-SNAPSHOT</version>
Review Comment:
And when adding new inner-project dependencies, intelliJ auto-fills it with the current version ... which you manually have to replace with the variable (I should probably add to the list of benefits ;-) )
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#discussion_r1359878037
##########
distribution/pom.xml:
##########
@@ -27,7 +27,43 @@
<version>1.3.0-SNAPSHOT</version>
</parent>
<artifactId>iotdb-distribution</artifactId>
+ <packaging>pom</packaging>
<name>IoTDB: Distribution</name>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.iotdb</groupId>
+ <artifactId>iotdb-server</artifactId>
+ <version>1.3.0-SNAPSHOT</version>
Review Comment:
The maven-release-plugin as well as them maven-versions-plugin perfectly handle this case ... there are issues with some non-standard plugins, that if the version is used in the configuration of there, that they are not found, so I strongly encourage using ${project.version} in plugin configurations, however for dependencies I have never seen these plugins miss anything.
##########
distribution/pom.xml:
##########
@@ -27,7 +27,43 @@
<version>1.3.0-SNAPSHOT</version>
</parent>
<artifactId>iotdb-distribution</artifactId>
+ <packaging>pom</packaging>
<name>IoTDB: Distribution</name>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.iotdb</groupId>
+ <artifactId>iotdb-server</artifactId>
+ <version>1.3.0-SNAPSHOT</version>
Review Comment:
The maven-release-plugin as well as the maven-versions-plugin perfectly handle this case ... there are issues with some non-standard plugins, that if the version is used in the configuration of there, that they are not found, so I strongly encourage using ${project.version} in plugin configurations, however for dependencies I have never seen these plugins miss anything.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "qiaojialin (via GitHub)" <gi...@apache.org>.
qiaojialin commented on code in PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#discussion_r1359871375
##########
distribution/pom.xml:
##########
@@ -27,7 +27,43 @@
<version>1.3.0-SNAPSHOT</version>
</parent>
<artifactId>iotdb-distribution</artifactId>
+ <packaging>pom</packaging>
<name>IoTDB: Distribution</name>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.iotdb</groupId>
+ <artifactId>iotdb-server</artifactId>
+ <version>1.3.0-SNAPSHOT</version>
Review Comment:
If the version could be changed in one mvn command, using real version is better.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#issuecomment-1763391210
On the first look it does seem as if this process adds a bit more work for the developer, which it does. However long-term it pays off 10 time as managing dependency problems becomes really easy and it has saved us many days of tracing down tricky problems.
For example in the current "Master" branch we use different versions of ratis ... unfortunately versions that are API incompatible. So depending on which modules get added to the distribution in which order will define which module works, which I don't know if we have encountered this problem before, however I am sure, if we had run into it ... that would have taken us quite some time to track down.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "HTHou (via GitHub)" <gi...@apache.org>.
HTHou commented on code in PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#discussion_r1359797069
##########
distribution/pom.xml:
##########
@@ -27,7 +27,43 @@
<version>1.3.0-SNAPSHOT</version>
</parent>
<artifactId>iotdb-distribution</artifactId>
+ <packaging>pom</packaging>
<name>IoTDB: Distribution</name>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.iotdb</groupId>
+ <artifactId>iotdb-server</artifactId>
+ <version>1.3.0-SNAPSHOT</version>
Review Comment:
<version>${project.version}</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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#discussion_r1359858939
##########
distribution/pom.xml:
##########
@@ -27,7 +27,43 @@
<version>1.3.0-SNAPSHOT</version>
</parent>
<artifactId>iotdb-distribution</artifactId>
+ <packaging>pom</packaging>
<name>IoTDB: Distribution</name>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.iotdb</groupId>
+ <artifactId>iotdb-server</artifactId>
+ <version>1.3.0-SNAPSHOT</version>
Review Comment:
And did you stop reviewing after this, or can I interpret it as that this is the only objection you have?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "SpriCoder (via GitHub)" <gi...@apache.org>.
SpriCoder commented on PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#issuecomment-1774374087
This pr cause the failure of manager loading in metric module. So it cause the unavailable of metric module. Let me take a look
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "qiaojialin (via GitHub)" <gi...@apache.org>.
qiaojialin merged PR #11150:
URL: https://github.com/apache/iotdb/pull/11150
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#issuecomment-1763390379
I have put a lot more strict rules in place ... these will prevent us from having diverging versions again and will inform us if there are CVEs reported for dependencies we use.
In general the rule is: don't define the versions of plugins and third-party dependencies locally, if you add a new third party dependency or plugin, add that to the pluginManagement or dependencyManagement of the main pom and then use notation without version in sub-modules.
Another rule is, that if you use something in your code, you need to add a dependency to that artifact in the correct scope, otherwise the build will fail. The reasoning behind this is, that if you rely on transitive dependencies, these possibly change without us noticing causing hard to debug problems.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "qiaojialin (via GitHub)" <gi...@apache.org>.
qiaojialin commented on PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#issuecomment-1764178119
So how does these rules work? Will they take effect when we do mvn package?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#discussion_r1359857523
##########
distribution/pom.xml:
##########
@@ -27,7 +27,43 @@
<version>1.3.0-SNAPSHOT</version>
</parent>
<artifactId>iotdb-distribution</artifactId>
+ <packaging>pom</packaging>
<name>IoTDB: Distribution</name>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.iotdb</groupId>
+ <artifactId>iotdb-server</artifactId>
+ <version>1.3.0-SNAPSHOT</version>
Review Comment:
Sorry .. shouldn't have replied on my phone ... especially if it resolved the discussion and threw away my text ... so here goes again:
Technically there's no difference between using the one or the other. I actually find it easier to type the real version when adding a new dependency than requiring 3 special characters. But beyond that when switching between versions and branches, I usually find it beneficial to use the real versions as then you don't need to scroll to the top to see which version we're in.
However if it's project consensus to use ${project.version}, I'll change it to that. If it hasn't been discussed, then I'd like to start a discussion to reach consensus on it.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "HTHou (via GitHub)" <gi...@apache.org>.
HTHou commented on code in PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#discussion_r1360061300
##########
distribution/pom.xml:
##########
@@ -27,7 +27,43 @@
<version>1.3.0-SNAPSHOT</version>
</parent>
<artifactId>iotdb-distribution</artifactId>
+ <packaging>pom</packaging>
<name>IoTDB: Distribution</name>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.iotdb</groupId>
+ <artifactId>iotdb-server</artifactId>
+ <version>1.3.0-SNAPSHOT</version>
Review Comment:
> And did you stop reviewing after this, or can I interpret it as that this is the only objection you have?
Yeah, it is the only question I have.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#issuecomment-1762963766
So I guess I'm done .... please ensure this PR can be merged ASAP, as keeping it up to date costs a lot of 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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor/managing dependencies [iotdb]
Posted by "qiaojialin (via GitHub)" <gi...@apache.org>.
qiaojialin commented on PR #11150:
URL: https://github.com/apache/iotdb/pull/11150#issuecomment-1763378558
Hi, after this pr, is there anything that contributors need to pay attention or execute when commit code?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org