ctubbsii commented on code in PR #110:
URL: https://github.com/apache/accumulo-access/pull/110#discussion_r2920721404
##########
modules/core/pom.xml:
##########
@@ -114,6 +118,10 @@
<classpath />
<argument>org.apache.accumulo.access.tests.AccessExpressionBenchmark</argument>
</arguments>
+ <environmentVariables>
+ <ENABLE_JFR>${benchmark.jfr}</ENABLE_JFR>
+ <ACCESS_BENCHMARK>${benchmark.methods}</ACCESS_BENCHMARK>
+ </environmentVariables>
Review Comment:
Instead of setting these environment variables from the command-line
provided `-D` system properties passed to Maven, it would be simpler to just
set them for the process (or export them). Having an indirect way to set an
environment variable via a system property is unnecessarily convoluted, if
that's all they are doing.
```java
mvn verify -Pbenchmark -Dbenchmark.jfr=true # sets system prop which sets env
ENABLE_JFR=true mvn verify -Pbenchmark # sets the env directly
```
However, it may be useful to reuse the properties to trigger the profile, as
in:
```java
mvn verify -Dbenchmark=.*
```
I think it's okay to merge this PR, and I can submit a proposed follow-on PR
to simplify this a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
