You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "rfscholte (via GitHub)" <gi...@apache.org> on 2024/08/20 13:45:55 UTC
[PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
rfscholte opened a new pull request, #13854:
URL: https://github.com/apache/pinot/pull/13854
…ge is done via codecov.io
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix`
3. `performance`
4. `ui`
5. `backward-incompat`
6. `release-notes` (**)
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `dependencies`
- `docker`
- `kubernetes`
- `observability`
- `security`
- `code-style`
- `extension-point`
- `refactor`
- `cleanup`
(**) Use `release-notes` label for scenarios like:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725313264
##########
.github/workflows/pinot_tests.yml:
##########
@@ -133,7 +133,7 @@ jobs:
MAVEN_OPTS: >
-Xmx2G -DskipShade -DfailIfNoTests=false -Dmaven.wagon.httpconnectionManager.ttlSeconds=25
-Dmaven.wagon.http.retryHandler.count=30 -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false
- -B -ntp
+ -B -ntp -P codecoverage
Review Comment:
I think these changes should be applied in https://github.com/apache/pinot/blob/5d58102d0ac7366d578ea1012ec125379ef70e7c/.github/workflows/scripts/pr-tests instead
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "rfscholte (via GitHub)" <gi...@apache.org>.
rfscholte commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1755074144
##########
pom.xml:
##########
@@ -354,6 +354,55 @@
<argLine>-Xms4g -Xmx4g -Dlog4j2.configurationFile=log4j2.xml</argLine>
</properties>
</profile>
+ <profile>
+ <id>codecoverage</id>
+ <build>
+ <pluginManagement>
+ <plugins>
+ <plugin>
+ <groupId>org.jacoco</groupId>
+ <artifactId>jacoco-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <!-- used when runnning jacoco:report-aggregate@report via commandline -->
+ <id>report</id>
+ <configuration>
+ <skip>true</skip> <!-- default for all modules, overwrite for root project -->
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </pluginManagement>
+ <plugins>
+ <plugin>
+ <groupId>org.jacoco</groupId>
+ <artifactId>jacoco-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>prepare-agent</id>
+ <goals>
+ <goal>prepare-agent</goal>
+ </goals>
+ <configuration>
+ <append>true</append>
+ <includes>
+ <include>org/apache/pinot/**/*</include>
+ </includes>
+ </configuration>
+ </execution>
+ <execution>
+ <id>report</id>
+ <inherited>false</inherited>
+ <configuration>
+ <skip>false</skip>
Review Comment:
let me verify if this is still required. If it was part of the buildcycle, you only want to execute this goal on the root, not on all the others. Here the default behavior of skipping is in plugin management, but with the execution+inherited=false you can configure plugins to do things that are not inherited by their children.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725095760
##########
pom.xml:
##########
@@ -2339,39 +2339,6 @@
</execution>
</executions>
</plugin>
- <plugin>
- <groupId>org.jacoco</groupId>
- <artifactId>jacoco-maven-plugin</artifactId>
- <version>0.8.12</version>
- <executions>
- <execution>
- <goals>
- <goal>prepare-agent</goal>
- </goals>
- </execution>
- <execution>
- <id>report</id>
- <phase>test</phase>
- <goals>
- <goal>report-aggregate</goal>
- </goals>
- <configuration>
- <includeCurrentProject>true</includeCurrentProject>
- </configuration>
- </execution>
- </executions>
- <configuration>
- <append>true</append>
- <includes>
- <include>org/apache/pinot/**/*</include>
- </includes>
- </configuration>
- </plugin>
- <plugin>
- <groupId>org.codehaus.mojo</groupId>
- <artifactId>sonar-maven-plugin</artifactId>
- <version>2.7.1</version>
- </plugin>
Review Comment:
Can we also specify sonar plugin in coverage profile? That way we can be sure the builds from GitHub Actions keep producing the same artifacts.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "rfscholte (via GitHub)" <gi...@apache.org>.
rfscholte commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725111020
##########
pom.xml:
##########
@@ -2339,39 +2339,6 @@
</execution>
</executions>
</plugin>
- <plugin>
- <groupId>org.jacoco</groupId>
- <artifactId>jacoco-maven-plugin</artifactId>
- <version>0.8.12</version>
- <executions>
- <execution>
- <goals>
- <goal>prepare-agent</goal>
- </goals>
- </execution>
- <execution>
- <id>report</id>
- <phase>test</phase>
- <goals>
- <goal>report-aggregate</goal>
- </goals>
- <configuration>
- <includeCurrentProject>true</includeCurrentProject>
- </configuration>
- </execution>
- </executions>
- <configuration>
- <append>true</append>
- <includes>
- <include>org/apache/pinot/**/*</include>
- </includes>
- </configuration>
- </plugin>
- <plugin>
- <groupId>org.codehaus.mojo</groupId>
- <artifactId>sonar-maven-plugin</artifactId>
- <version>2.7.1</version>
- </plugin>
Review Comment:
AFAIK the sonar-plugin is only used to communicate with SonarQube, but if it does more, the plugin should be moved to the codecoverage profile
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "rfscholte (via GitHub)" <gi...@apache.org>.
rfscholte commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1723516284
##########
pom.xml:
##########
@@ -2339,39 +2339,6 @@
</execution>
</executions>
</plugin>
- <plugin>
- <groupId>org.jacoco</groupId>
- <artifactId>jacoco-maven-plugin</artifactId>
- <version>0.8.12</version>
- <executions>
- <execution>
- <goals>
- <goal>prepare-agent</goal>
- </goals>
- </execution>
- <execution>
- <id>report</id>
- <phase>test</phase>
- <goals>
- <goal>report-aggregate</goal>
- </goals>
- <configuration>
- <includeCurrentProject>true</includeCurrentProject>
- </configuration>
- </execution>
- </executions>
- <configuration>
- <append>true</append>
- <includes>
- <include>org/apache/pinot/**/*</include>
- </includes>
- </configuration>
- </plugin>
- <plugin>
- <groupId>org.codehaus.mojo</groupId>
- <artifactId>sonar-maven-plugin</artifactId>
- <version>2.7.1</version>
- </plugin>
Review Comment:
i couldn't find the usage of jacoco by codecov, but if that's the case, I need to adjust this. It should at least move to a dedicated maven profile.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725384006
##########
.github/workflows/pinot_tests.yml:
##########
@@ -133,7 +133,7 @@ jobs:
MAVEN_OPTS: >
-Xmx2G -DskipShade -DfailIfNoTests=false -Dmaven.wagon.httpconnectionManager.ttlSeconds=25
-Dmaven.wagon.http.retryHandler.count=30 -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false
- -B -ntp
+ -B -ntp -P codecoverage
Review Comment:
In that folder there are 4 scripts. Each one call maven at least once. There is where we have been setting the profiles.
I don't have a strong opinion between applying the profile in these scripts or here.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725097069
##########
pom.xml:
##########
@@ -2431,6 +2432,7 @@
<!-- Auto-generated target directories -->
<exclude>target/**</exclude>
<exclude>pinot-*/target/**</exclude>
+ <exclude>build/**</exclude> <!-- symbolic link pointing to pinot/pinot-distribution/target/apache-pinot-1.3.0-SNAPSHOT-bin/apache-pinot-1.3.0-SNAPSHOT-bin -->
Review Comment:
Note for reviewers: This symlink is not followed in Windows, so the license plugin consider it an actual file without valid license 😆
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1741500704
##########
.github/workflows/scripts/pr-tests/.pinot_tests_unit.sh:
##########
@@ -29,24 +29,24 @@ netstat -i
# - TEST_SET#1 runs install and test together so the module list must ensure no additional modules were tested
# due to the -am flag (include dependency modules)
if [ "$RUN_TEST_SET" == "1" ]; then
- mvn test -T 16 \
+ mvn test jacoco:report-aggregate -T 16 \
-pl 'pinot-spi' \
-pl 'pinot-segment-spi' \
-pl 'pinot-common' \
-pl ':pinot-yammer' \
-pl 'pinot-core' \
-pl 'pinot-query-planner' \
-pl 'pinot-query-runtime' \
- -P github-actions,no-integration-tests || exit 1
+ -P github-actions,codecoverage,no-integration-tests || exit 1
Review Comment:
I don't think this is working as expected. Given we are providing a inclusive list of projects and `-pl reports` is not included, we are not actually executing the aggregate defined in reports/pom.xml. What we are doing is to directly run `jacoco:report-aggregate` on each project, which I don't think it is super useful.
We should also apply this change in the integration tests file (`.pinot_tests_integration.sh`)
Am I 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "rfscholte (via GitHub)" <gi...@apache.org>.
rfscholte commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725408612
##########
.github/workflows/pinot_tests.yml:
##########
@@ -133,7 +133,7 @@ jobs:
MAVEN_OPTS: >
-Xmx2G -DskipShade -DfailIfNoTests=false -Dmaven.wagon.httpconnectionManager.ttlSeconds=25
-Dmaven.wagon.http.retryHandler.count=30 -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false
- -B -ntp
+ -B -ntp -P codecoverage
Review Comment:
and that is true: `-B` is not a JVM options but an application argument, while `MAVEN_OPTS` should only contain JVM options. Makes me wonder how this could have ever worked.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1752841462
##########
pom.xml:
##########
@@ -2048,6 +2097,11 @@
<artifactId>jacoco-maven-plugin</artifactId>
<version>0.8.12</version>
</plugin>
+ <plugin>
Review Comment:
Sonar was removed in #13878. I don't think we need to introduce it back here
##########
pinot-integration-tests/pom.xml:
##########
@@ -36,6 +36,25 @@
</properties>
<profiles>
+ <profile>
Review Comment:
Do we need this section?
##########
pom.xml:
##########
@@ -354,6 +354,55 @@
<argLine>-Xms4g -Xmx4g -Dlog4j2.configurationFile=log4j2.xml</argLine>
</properties>
</profile>
+ <profile>
+ <id>codecoverage</id>
+ <build>
+ <pluginManagement>
+ <plugins>
+ <plugin>
+ <groupId>org.jacoco</groupId>
+ <artifactId>jacoco-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <!-- used when runnning jacoco:report-aggregate@report via commandline -->
+ <id>report</id>
+ <configuration>
+ <skip>true</skip> <!-- default for all modules, overwrite for root project -->
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </pluginManagement>
+ <plugins>
+ <plugin>
+ <groupId>org.jacoco</groupId>
+ <artifactId>jacoco-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>prepare-agent</id>
+ <goals>
+ <goal>prepare-agent</goal>
+ </goals>
+ <configuration>
+ <append>true</append>
+ <includes>
+ <include>org/apache/pinot/**/*</include>
+ </includes>
+ </configuration>
+ </execution>
+ <execution>
+ <id>report</id>
+ <inherited>false</inherited>
+ <configuration>
+ <skip>false</skip>
Review Comment:
I don't fully follow this. If we override it here, do we need to declare it in the plugin management?
##########
reports/pom.xml:
##########
@@ -0,0 +1,355 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot</artifactId>
+ <version>1.3.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>reports</artifactId>
+ <packaging>pom</packaging>
+
+ <name>Pinot Reports</name>
+ <description>This module will be used to generate aggregated reports.
+ This cannot be done in the root pom, because that one is executed before all other modules.
+ Adding dependencies here tricks jacoco to generate the right aggregate-report.
+ https://github.com/jacoco/jacoco/wiki/MavenMultiModule
+ </description>
+
+ <properties>
+ <pinot.root>${basedir}/..</pinot.root>
+ </properties>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>default-cli</id>
+ <phase>none</phase> <!-- this project should only report, not fail; -->
+ </execution>
+ </executions>
+ </plugin>
+ <plugin>
+ <groupId>org.jacoco</groupId>
+ <artifactId>jacoco-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>report</id>
+ <phase>test</phase>
+ <goals>
+ <goal>report-aggregate</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-install-plugin</artifactId>
+ <configuration>
+ <skip>true</skip>
+ </configuration>
+ </plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-deploy-plugin</artifactId>
+ <configuration>
+ <skip>true</skip>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+
+ <dependencies>
+ <!-- COPIED FROM root pom minus version -->
+
+ <!-- Pinot Modules -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spi</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-segment-spi</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-common</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-segment-local</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-query-planner</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-query-runtime</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-controller</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-broker</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-server</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-minion</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-java-client</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-jdbc-client</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-tools</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-perf</artifactId>
+ </dependency>
+
+ <!-- Pinot Plug-in Modules -->
+ <!-- Batch Ingestion -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-batch-ingestion-common</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-batch-ingestion-hadoop</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-batch-ingestion-spark-2.4</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-batch-ingestion-spark-3</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-batch-ingestion-standalone</artifactId>
+ </dependency>
+ <!-- Stream Ingestion -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-kafka-base</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-kafka-2.0</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-kinesis</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-pulsar</artifactId>
+ </dependency>
+ <!-- Input Format -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-avro-base</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-avro</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-confluent-avro</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-orc</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-parquet</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-json</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-csv</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-thrift</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-protobuf</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-clp-log</artifactId>
+ </dependency>
+ <!-- File System -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-hdfs</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-s3</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-gcs</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-adls</artifactId>
+ </dependency>
+ <!-- Environment -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-azure</artifactId>
+ </dependency>
+ <!-- Metrics -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-yammer</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-dropwizard</artifactId>
+ </dependency>
+ <!-- Minion Tasks -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-minion-builtin-tasks</artifactId>
+ </dependency>
+ <!-- Segment Writer -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-segment-writer-file-based</artifactId>
+ </dependency>
+ <!-- Segment Uploader -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-segment-uploader-default</artifactId>
+ </dependency>
+
+ <!-- Pinot Connector Modules -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spark-common</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spark-2-connector</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spark-3-connector</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-flink-connector</artifactId>
+ </dependency>
+
+ <!-- Pinot Test Jars -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spi</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-common</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-segment-local</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-core</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-query-planner</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-controller</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-broker</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-server</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-minion</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-integration-test-base</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-integration-tests</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ </dependencies>
+</project>
Review Comment:
(nit) Add an empty line in the end
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1723481958
##########
pom.xml:
##########
@@ -2339,39 +2339,6 @@
</execution>
</executions>
</plugin>
- <plugin>
- <groupId>org.jacoco</groupId>
- <artifactId>jacoco-maven-plugin</artifactId>
- <version>0.8.12</version>
- <executions>
- <execution>
- <goals>
- <goal>prepare-agent</goal>
- </goals>
- </execution>
- <execution>
- <id>report</id>
- <phase>test</phase>
- <goals>
- <goal>report-aggregate</goal>
- </goals>
- <configuration>
- <includeCurrentProject>true</includeCurrentProject>
- </configuration>
- </execution>
- </executions>
- <configuration>
- <append>true</append>
- <includes>
- <include>org/apache/pinot/**/*</include>
- </includes>
- </configuration>
- </plugin>
- <plugin>
- <groupId>org.codehaus.mojo</groupId>
- <artifactId>sonar-maven-plugin</artifactId>
- <version>2.7.1</version>
- </plugin>
Review Comment:
+1 to gonzalo's suggestion. Jacoco generates a lot of HTML files and reports that are really not needed. It only runs during the test phase so most of the time doesn't hamper local development 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "rfscholte (via GitHub)" <gi...@apache.org>.
rfscholte commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1754947033
##########
pinot-integration-tests/pom.xml:
##########
@@ -36,6 +36,25 @@
</properties>
<profiles>
+ <profile>
Review Comment:
I've adjusted it a bit, by adding the goal+phase, so it can become part of the build lifecycle. For the integration-test that's ok, as it is only 1 module.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1755485136
##########
.github/workflows/scripts/pr-tests/.pinot_tests_unit.sh:
##########
@@ -48,5 +48,7 @@ if [ "$RUN_TEST_SET" == "2" ]; then
-pl '!pinot-query-planner' \
-pl '!pinot-query-runtime' \
-pl '!:pinot-yammer' \
- -P github-actions,no-integration-tests || exit 1
+ -P github-actions,codecoverage,no-integration-tests || exit 1
fi
+
+mvn -N jacoco:report-aggregate@report -P codecoverage || exit 1
Review Comment:
(nit) Empty line in the end
We should probably add a rule to enforce 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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725384804
##########
.github/workflows/pinot_tests.yml:
##########
@@ -133,7 +133,7 @@ jobs:
MAVEN_OPTS: >
-Xmx2G -DskipShade -DfailIfNoTests=false -Dmaven.wagon.httpconnectionManager.ttlSeconds=25
-Dmaven.wagon.http.retryHandler.count=30 -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false
- -B -ntp
+ -B -ntp -P codecoverage
Review Comment:
But after this change it looks like all executions are failing because `-B is not recognized as JVM option`
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725097069
##########
pom.xml:
##########
@@ -2431,6 +2432,7 @@
<!-- Auto-generated target directories -->
<exclude>target/**</exclude>
<exclude>pinot-*/target/**</exclude>
+ <exclude>build/**</exclude> <!-- symbolic link pointing to pinot/pinot-distribution/target/apache-pinot-1.3.0-SNAPSHOT-bin/apache-pinot-1.3.0-SNAPSHOT-bin -->
Review Comment:
Note for reviewers: This symlink is not being understood by Windows, so the license plugin consider it an actual file without valid license 😆
##########
pom.xml:
##########
@@ -2431,6 +2432,7 @@
<!-- Auto-generated target directories -->
<exclude>target/**</exclude>
<exclude>pinot-*/target/**</exclude>
+ <exclude>build/**</exclude> <!-- symbolic link pointing to pinot/pinot-distribution/target/apache-pinot-1.3.0-SNAPSHOT-bin/apache-pinot-1.3.0-SNAPSHOT-bin -->
Review Comment:
Note for reviewers: This symlink is not understood by Windows, so the license plugin consider it an actual file without valid license 😆
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "rfscholte (via GitHub)" <gi...@apache.org>.
rfscholte commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1741518838
##########
.github/workflows/scripts/pr-tests/.pinot_tests_unit.sh:
##########
@@ -29,24 +29,24 @@ netstat -i
# - TEST_SET#1 runs install and test together so the module list must ensure no additional modules were tested
# due to the -am flag (include dependency modules)
if [ "$RUN_TEST_SET" == "1" ]; then
- mvn test -T 16 \
+ mvn test jacoco:report-aggregate -T 16 \
-pl 'pinot-spi' \
-pl 'pinot-segment-spi' \
-pl 'pinot-common' \
-pl ':pinot-yammer' \
-pl 'pinot-core' \
-pl 'pinot-query-planner' \
-pl 'pinot-query-runtime' \
- -P github-actions,no-integration-tests || exit 1
+ -P github-actions,codecoverage,no-integration-tests || exit 1
Review Comment:
almost right :) The root issue is the split of modules to build/test, while the reports needs to have all modules. So the reports-module fails if dependencies are missing. By putting `jacoco:report-aggregate` on the commandline, it is execute as last (which is the opposite of current behavior: run aggregate as first). With inherited=false we can separate the root module from its child modules, but I guess I need to configure the skip correctly for both.
If this works, I suggest to remove the reports-module as it will only lead to confusion.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #13854:
URL: https://github.com/apache/pinot/pull/13854
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #13854:
URL: https://github.com/apache/pinot/pull/13854#issuecomment-2318044437
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/13854?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 55.77%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`f9a39dc`)](https://app.codecov.io/gh/apache/pinot/commit/f9a39dca4da35ed98ec8c371f6199fcdc3415bfd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 958 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (f9a39dc). Click for more details.
>
> <details><summary>HEAD has 48 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (f9a39dc) |
>|------|------|------|
>|integration|7|0|
>|integration2|3|0|
>|temurin|12|2|
>|java-21|7|2|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|1|
>|unittests|5|2|
>|unittests1|2|0|
>|java-11|5|0|
>|unittests2|3|2|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #13854 +/- ##
=============================================
- Coverage 61.75% 55.77% -5.98%
- Complexity 207 4171 +3964
=============================================
Files 2436 382 -2054
Lines 133233 26394 -106839
Branches 20636 3724 -16912
=============================================
- Hits 82274 14722 -67552
+ Misses 44911 10458 -34453
+ Partials 6048 1214 -4834
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/13854/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/13854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/13854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/13854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/13854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/13854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/13854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.77% <ø> (-5.85%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.74% <ø> (-6.01%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.77% <ø> (+28.05%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/13854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.77% <ø> (-5.98%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/13854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.77% <ø> (-5.97%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/13854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/13854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.77% <ø> (+28.04%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/13854?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1723451316
##########
pom.xml:
##########
@@ -2339,39 +2339,6 @@
</execution>
</executions>
</plugin>
- <plugin>
- <groupId>org.jacoco</groupId>
- <artifactId>jacoco-maven-plugin</artifactId>
- <version>0.8.12</version>
- <executions>
- <execution>
- <goals>
- <goal>prepare-agent</goal>
- </goals>
- </execution>
- <execution>
- <id>report</id>
- <phase>test</phase>
- <goals>
- <goal>report-aggregate</goal>
- </goals>
- <configuration>
- <includeCurrentProject>true</includeCurrentProject>
- </configuration>
- </execution>
- </executions>
- <configuration>
- <append>true</append>
- <includes>
- <include>org/apache/pinot/**/*</include>
- </includes>
- </configuration>
- </plugin>
- <plugin>
- <groupId>org.codehaus.mojo</groupId>
- <artifactId>sonar-maven-plugin</artifactId>
- <version>2.7.1</version>
- </plugin>
Review Comment:
I think this is incorrect. What we sent to codecov is the jacoco artifacts.
We may not need to run `report-aggregate` and instead run `report`, but that in fact will give us incorrect metric values given some tests classes are actually tested from different projects.
Instead of removing the plugin, what I suggest is to move jacoco to a specific profile we can enable during CI. That way developers should not be affected in their normal compilations.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725092488
##########
pom.xml:
##########
@@ -2339,39 +2339,6 @@
</execution>
</executions>
</plugin>
- <plugin>
- <groupId>org.jacoco</groupId>
- <artifactId>jacoco-maven-plugin</artifactId>
- <version>0.8.12</version>
- <executions>
- <execution>
- <goals>
- <goal>prepare-agent</goal>
- </goals>
- </execution>
- <execution>
- <id>report</id>
- <phase>test</phase>
- <goals>
- <goal>report-aggregate</goal>
- </goals>
- <configuration>
- <includeCurrentProject>true</includeCurrentProject>
- </configuration>
- </execution>
- </executions>
- <configuration>
- <append>true</append>
- <includes>
- <include>org/apache/pinot/**/*</include>
- </includes>
- </configuration>
- </plugin>
- <plugin>
- <groupId>org.codehaus.mojo</groupId>
- <artifactId>sonar-maven-plugin</artifactId>
- <version>2.7.1</version>
- </plugin>
Review Comment:
Codecov github actions supports a list of files to upload but we don't use that because by default codecov looks for all jacoco.xml files in the file tree. You can see that in action in https://github.com/apache/pinot/actions/runs/10482523110/job/29033779007?pr=13859 clicking in `Upload coverage to codecov`
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "rfscholte (via GitHub)" <gi...@apache.org>.
rfscholte commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725107694
##########
pom.xml:
##########
@@ -2431,6 +2432,7 @@
<!-- Auto-generated target directories -->
<exclude>target/**</exclude>
<exclude>pinot-*/target/**</exclude>
+ <exclude>build/**</exclude> <!-- symbolic link pointing to pinot/pinot-distribution/target/apache-pinot-1.3.0-SNAPSHOT-bin/apache-pinot-1.3.0-SNAPSHOT-bin -->
Review Comment:
let me remove this one, Windows is a separate topic ;)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "rfscholte (via GitHub)" <gi...@apache.org>.
rfscholte commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1754970138
##########
reports/pom.xml:
##########
@@ -0,0 +1,355 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot</artifactId>
+ <version>1.3.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>reports</artifactId>
+ <packaging>pom</packaging>
+
+ <name>Pinot Reports</name>
+ <description>This module will be used to generate aggregated reports.
+ This cannot be done in the root pom, because that one is executed before all other modules.
+ Adding dependencies here tricks jacoco to generate the right aggregate-report.
+ https://github.com/jacoco/jacoco/wiki/MavenMultiModule
+ </description>
+
+ <properties>
+ <pinot.root>${basedir}/..</pinot.root>
+ </properties>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>default-cli</id>
+ <phase>none</phase> <!-- this project should only report, not fail; -->
+ </execution>
+ </executions>
+ </plugin>
+ <plugin>
+ <groupId>org.jacoco</groupId>
+ <artifactId>jacoco-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>report</id>
+ <phase>test</phase>
+ <goals>
+ <goal>report-aggregate</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-install-plugin</artifactId>
+ <configuration>
+ <skip>true</skip>
+ </configuration>
+ </plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-deploy-plugin</artifactId>
+ <configuration>
+ <skip>true</skip>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+
+ <dependencies>
+ <!-- COPIED FROM root pom minus version -->
+
+ <!-- Pinot Modules -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spi</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-segment-spi</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-common</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-segment-local</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-query-planner</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-query-runtime</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-controller</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-broker</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-server</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-minion</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-java-client</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-jdbc-client</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-tools</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-perf</artifactId>
+ </dependency>
+
+ <!-- Pinot Plug-in Modules -->
+ <!-- Batch Ingestion -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-batch-ingestion-common</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-batch-ingestion-hadoop</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-batch-ingestion-spark-2.4</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-batch-ingestion-spark-3</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-batch-ingestion-standalone</artifactId>
+ </dependency>
+ <!-- Stream Ingestion -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-kafka-base</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-kafka-2.0</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-kinesis</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-pulsar</artifactId>
+ </dependency>
+ <!-- Input Format -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-avro-base</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-avro</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-confluent-avro</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-orc</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-parquet</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-json</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-csv</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-thrift</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-protobuf</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-clp-log</artifactId>
+ </dependency>
+ <!-- File System -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-hdfs</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-s3</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-gcs</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-adls</artifactId>
+ </dependency>
+ <!-- Environment -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-azure</artifactId>
+ </dependency>
+ <!-- Metrics -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-yammer</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-dropwizard</artifactId>
+ </dependency>
+ <!-- Minion Tasks -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-minion-builtin-tasks</artifactId>
+ </dependency>
+ <!-- Segment Writer -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-segment-writer-file-based</artifactId>
+ </dependency>
+ <!-- Segment Uploader -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-segment-uploader-default</artifactId>
+ </dependency>
+
+ <!-- Pinot Connector Modules -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spark-common</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spark-2-connector</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spark-3-connector</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-flink-connector</artifactId>
+ </dependency>
+
+ <!-- Pinot Test Jars -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spi</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-common</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-segment-local</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-core</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-query-planner</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-controller</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-broker</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-server</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-minion</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-integration-test-base</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-integration-tests</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ </dependencies>
+</project>
Review Comment:
I've removed this file. It is not giving the result I wanted. Instead of resolving the reports as part of the lifecycle, I've adjusted the scripts to call maven a second time to generate the report.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "rfscholte (via GitHub)" <gi...@apache.org>.
rfscholte commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725330106
##########
.github/workflows/pinot_tests.yml:
##########
@@ -133,7 +133,7 @@ jobs:
MAVEN_OPTS: >
-Xmx2G -DskipShade -DfailIfNoTests=false -Dmaven.wagon.httpconnectionManager.ttlSeconds=25
-Dmaven.wagon.http.retryHandler.count=30 -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false
- -B -ntp
+ -B -ntp -P codecoverage
Review Comment:
I don't see where. Is there one specific build that is responsible for the codecoverage reports?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "rfscholte (via GitHub)" <gi...@apache.org>.
rfscholte commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725122532
##########
pom.xml:
##########
@@ -2339,39 +2373,6 @@
</execution>
</executions>
</plugin>
- <plugin>
- <groupId>org.jacoco</groupId>
- <artifactId>jacoco-maven-plugin</artifactId>
- <version>0.8.12</version>
- <executions>
- <execution>
- <goals>
- <goal>prepare-agent</goal>
- </goals>
- </execution>
- <execution>
- <id>report</id>
- <phase>test</phase>
- <goals>
- <goal>report-aggregate</goal>
- </goals>
- <configuration>
- <includeCurrentProject>true</includeCurrentProject>
- </configuration>
- </execution>
- </executions>
- <configuration>
- <append>true</append>
- <includes>
- <include>org/apache/pinot/**/*</include>
- </includes>
- </configuration>
- </plugin>
- <plugin>
Review Comment:
@gortiz this only locked the version of the plugin, I don't see it anywhere being used/called
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725097069
##########
pom.xml:
##########
@@ -2431,6 +2432,7 @@
<!-- Auto-generated target directories -->
<exclude>target/**</exclude>
<exclude>pinot-*/target/**</exclude>
+ <exclude>build/**</exclude> <!-- symbolic link pointing to pinot/pinot-distribution/target/apache-pinot-1.3.0-SNAPSHOT-bin/apache-pinot-1.3.0-SNAPSHOT-bin -->
Review Comment:
Note for reviewers: This symlink is not being honored in Windows, so the license plugin consider it an actual file without valid license 😆
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Remove both jacoco-maven-plugin and sonar-maven-plugin, as codecovera… [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1725177327
##########
pom.xml:
##########
@@ -2339,39 +2339,6 @@
</execution>
</executions>
</plugin>
- <plugin>
- <groupId>org.jacoco</groupId>
- <artifactId>jacoco-maven-plugin</artifactId>
- <version>0.8.12</version>
- <executions>
- <execution>
- <goals>
- <goal>prepare-agent</goal>
- </goals>
- </execution>
- <execution>
- <id>report</id>
- <phase>test</phase>
- <goals>
- <goal>report-aggregate</goal>
- </goals>
- <configuration>
- <includeCurrentProject>true</includeCurrentProject>
- </configuration>
- </execution>
- </executions>
- <configuration>
- <append>true</append>
- <includes>
- <include>org/apache/pinot/**/*</include>
- </includes>
- </configuration>
- </plugin>
- <plugin>
- <groupId>org.codehaus.mojo</groupId>
- <artifactId>sonar-maven-plugin</artifactId>
- <version>2.7.1</version>
- </plugin>
Review Comment:
@xiangfu0 @jfim Do you think we can remove the sonar plugin?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org