You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/07/31 02:18:52 UTC
[GitHub] [accumulo] EdColeman opened a new pull request #1667: Fix #1663 - Missing metrics files running CI tests
EdColeman opened a new pull request #1667:
URL: https://github.com/apache/accumulo/pull/1667
- excluded hadoop-metrics2-accumulo.properties from jar plugin
- added gc metrics config sample to template
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a change in pull request #1667: Fix #1663 - Missing metrics files running CI tests
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1667:
URL: https://github.com/apache/accumulo/pull/1667#discussion_r464606937
##########
File path: test/pom.xml
##########
@@ -270,6 +270,15 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <configuration>
+ <excludes>
+ <exclude>**/hadoop-metrics2-accumulo.properties</exclude>
Review comment:
Hit commit via ui, becuase - sure that's reasonable - and then decided to test - oops. Anyway I don't think that works as intended - might be src is assumed? Anyway, checking now...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a change in pull request #1667: Fix #1663 - Missing metrics files running CI tests
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1667:
URL: https://github.com/apache/accumulo/pull/1667#discussion_r463390971
##########
File path: pom.xml
##########
@@ -658,6 +658,9 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-jar-plugin</artifactId>
<configuration>
+ <excludes>
+ <exclude>**/hadoop-metrics2-accumulo.properties</exclude>
+ </excludes>
Review comment:
Should I add a complete maven-jar-plugin to the test module pom? I looked there but I did not see one configured. I thought about it, but was unsure if a new plugin configuration or promoting it to the parent would be preferred.
Having it in the parent does have a side benefit that if any module ends up needing a metrics file for testing it would be excluded by default.
It's your call as to the preferred approach.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #1667: Fix #1663 - Missing metrics files running CI tests
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #1667:
URL: https://github.com/apache/accumulo/pull/1667
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1667: Fix #1663 - Missing metrics files running CI tests
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1667:
URL: https://github.com/apache/accumulo/pull/1667#discussion_r464545302
##########
File path: test/pom.xml
##########
@@ -270,6 +270,15 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <configuration>
+ <excludes>
+ <exclude>**/hadoop-metrics2-accumulo.properties</exclude>
Review comment:
It occurs to me... does this still allow metrics to work when we're doing testing? If it's not in the jar, how does it get on the class path?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1667: Fix #1663 - Missing metrics files running CI tests
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1667:
URL: https://github.com/apache/accumulo/pull/1667#discussion_r463380223
##########
File path: pom.xml
##########
@@ -658,6 +658,9 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-jar-plugin</artifactId>
<configuration>
+ <excludes>
+ <exclude>**/hadoop-metrics2-accumulo.properties</exclude>
+ </excludes>
Review comment:
This should probably exist only in the test module, rather than in the root pom.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a change in pull request #1667: Fix #1663 - Missing metrics files running CI tests
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1667:
URL: https://github.com/apache/accumulo/pull/1667#discussion_r464648883
##########
File path: test/pom.xml
##########
@@ -270,6 +270,15 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <configuration>
+ <excludes>
+ <exclude>**/hadoop-metrics2-accumulo.properties</exclude>
Review comment:
So I basically tried more restrictive paths combinations and **/filename seems to be the "most" specific that actually excluded the file from the created jar - basically revered the UI generated commit. I examined the jars created with the current changes and run -Psunny and things look okay as it stands now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1667: Fix #1663 - Missing metrics files running CI tests
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1667:
URL: https://github.com/apache/accumulo/pull/1667#discussion_r463874451
##########
File path: pom.xml
##########
@@ -658,6 +658,9 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-jar-plugin</artifactId>
<configuration>
+ <excludes>
+ <exclude>**/hadoop-metrics2-accumulo.properties</exclude>
+ </excludes>
Review comment:
Placing the plugin section in the test module pom with this configured there would be preferred, so it doesn't unintentionally apply to other modules, and also so it's more visible to people modifying the test module in the future, in case they modify it in a way that is impacted by this configuration.
Other modules wouldn't be affected anyway, since they would use `src/test/resources`. The only reason the test module is affected is because it uses `src/main/resources`. If we actually wanted to intentionally put this file in another module's `src/main/resources`, we wouldn't want this configuration to accidentally omit 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1667: Fix #1663 - Missing metrics files running CI tests
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1667:
URL: https://github.com/apache/accumulo/pull/1667#discussion_r464589176
##########
File path: test/pom.xml
##########
@@ -270,6 +270,15 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <configuration>
+ <excludes>
+ <exclude>**/hadoop-metrics2-accumulo.properties</exclude>
Review comment:
`maven-jar-plugin` runs during the `package` phase of the default lifecycle. Usually, the `maven-assembly-plugin` is also configured to run during the `package` phase, but only if a user manually configured it, since it's not bound to the default lifecycle by default.
If our tests pass, I have no problem excluding it from the jar. Like you said, our metrics file is set for target directory, specifically, so it's clear it's only for our tests anyway. And, if our test code were reused, it wouldn't be hard for the caller to provide its own metrics config file.
I would avoid `/tmp`. It's best to use the `target/` directory, and write to no other directories in a Maven build.
My final point of feedback, then, is just a suggestion to use the path explicitly, rather than wildcards:
```suggestion
<exclude>src/main/resources/hadoop-metrics2-accumulo.properties</exclude>
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1667: Fix #1663 - Missing metrics files running CI tests
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1667:
URL: https://github.com/apache/accumulo/pull/1667#discussion_r463874738
##########
File path: test/pom.xml
##########
@@ -270,6 +270,21 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <configuration>
+ <excludes>
+ <exclude>**/hadoop-metrics2-accumulo.properties</exclude>
+ </excludes>
+ <archive>
+ <manifestEntries>
+ <Sealed>true</Sealed>
+ <Implementation-Build>${mvngit.commit.id}</Implementation-Build>
+ </manifestEntries>
+ </archive>
Review comment:
This can be omitted, as it would be merged with the configuration from the parent anyway. We only need to specify the portions that we wish to override in here.
```suggestion
</excludes>
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1667: Fix #1663 - Missing metrics files running CI tests
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1667:
URL: https://github.com/apache/accumulo/pull/1667#discussion_r464834995
##########
File path: test/pom.xml
##########
@@ -270,6 +270,15 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <configuration>
+ <excludes>
+ <exclude>**/hadoop-metrics2-accumulo.properties</exclude>
Review comment:
I tested the following more narrow construction, and it seems to work fine (see my last comment for why):
```suggestion
<exclude>hadoop-metrics2-accumulo.properties</exclude>
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a change in pull request #1667: Fix #1663 - Missing metrics files running CI tests
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1667:
URL: https://github.com/apache/accumulo/pull/1667#discussion_r464580459
##########
File path: test/pom.xml
##########
@@ -270,6 +270,15 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <configuration>
+ <excludes>
+ <exclude>**/hadoop-metrics2-accumulo.properties</exclude>
Review comment:
I was under the impression that the maven-jar-plugin would be called during assembly (package?) If you build / test under maven, the property file will still be there. So, any testing that uses classes / files under ..../target/... should still work.
You are correct though, if you tried to run it tests using just the jars - unless you provided a metrics configuration file with metrics output configured, then the tests that check those files would fail. If you go to the trouble to run the tests outside of maven using a command line, then if you got that far, adding a suitable config file may not be much of a hurdle and forces you to provide a configuration that you want for your needs.
One issue is that the metrics config file is specifying ./target/name as a know location for the output files - this was so that mvn clean would remove them and they would not be left hanging around.
I suppose that if there was another well know location that would always be suitable for writes, then that could be used instead. Sounds like just specified /tmp, but that's making an assumption that /tmp would always be suitable - for example would docker / containers / aws / ... prefer not to use /tmp but something else? And then there's the clean-up - would need something besides (or modification) mvn clean. Sure, /tmp may eventually get cleaned, but seems good practice not to depend on that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1667: Fix #1663 - Missing metrics files running CI tests
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1667:
URL: https://github.com/apache/accumulo/pull/1667#discussion_r464615274
##########
File path: test/pom.xml
##########
@@ -270,6 +270,15 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <configuration>
+ <excludes>
+ <exclude>**/hadoop-metrics2-accumulo.properties</exclude>
Review comment:
Oh! The jar plugin's base directory might be target/classes, where the compiler plugin leaves class files and the resource plugin leaves resources. It might just work with the file name only, since this resource isn't in a subdirectory. Not sure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org