[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1972: SOLR-14915: Prometheus-exporter does not depend on Solr-core any longer

[GitBox <gi...@apache.org>]

dsmiley commented on a change in pull request #1972:
URL: https://github.com/apache/lucene-solr/pull/1972#discussion_r523776751



##########
File path: gradle/solr/packaging.gradle
##########
@@ -76,7 +76,8 @@ configure(allprojects.findAll {project -> project.path.startsWith(":solr:contrib
             return true
           }
         }
-        return externalLibs - configurations.solrPlatformLibs
+        // libExt has logging libs, which we don't want.  Lets users decide what they want.
+        return externalLibs - configurations.solrPlatformLibs - project(':solr:server').configurations.getByName('libExt')

Review comment:
       It might be considered hacky to reference a configuration of a specific sub-project.  Maybe libExt could be declared more top-level and be called "loggingLibs" or something.

##########
File path: solr/contrib/prometheus-exporter/bin/solr-exporter
##########
@@ -123,8 +111,6 @@ else
     GC_TUNE="$GC_TUNE"
 fi
 
-EXTRA_JVM_ARGUMENTS="-Dlog4j.configurationFile=file:"$BASEDIR"/../../server/resources/log4j2-console.xml"

Review comment:
       Instead, /conf is put on the classpath, and thus "log4j2.xml" is loaded automatically based on Log4j2's automatic resolution.

##########
File path: solr/contrib/prometheus-exporter/build.gradle
##########
@@ -31,17 +30,57 @@ dependencies {
     exclude group: "org.jruby.joni", module: "joni"
   })
   implementation ('net.sourceforge.argparse4j:argparse4j')
+  implementation ('com.github.ben-manes.caffeine:caffeine', {
+    exclude group: "org.checkerframework", module: "checker-qual"
+    exclude group: "com.google.errorprone", module: "error_prone_annotations"
+  })
 
-  testImplementation ('org.apache.httpcomponents:httpcore')
-  testImplementation ('org.eclipse.jetty:jetty-servlet')
+  runtimeOnly 'org.apache.logging.log4j:log4j-api'

Review comment:
       Logging dependencies in Java is a pain.  In a separate issue/PR, we should explore this approach: https://blog.gradle.org/addressing-logging-complexity-capabilities

##########
File path: solr/contrib/prometheus-exporter/src/java/org/apache/solr/prometheus/exporter/SolrExporter.java
##########
@@ -68,7 +66,7 @@
   private static final String[] ARG_CONFIG_FLAGS = {"-f", "--config-file"};
   private static final String ARG_CONFIG_METAVAR = "CONFIG";
   private static final String ARG_CONFIG_DEST = "configFile";
-  private static final String ARG_CONFIG_DEFAULT = "solr-exporter-config.xml";
+  private static final String ARG_CONFIG_DEFAULT = "conf/solr-exporter-config.xml";

Review comment:
       @HoustonPutman you removed the conf/ and I'm putting it back.  I'm not sure why it worked with it gone.  It may be related to other changes in this PR that it's needed again.  I tested that this works here both with `gw run` and via executing normally from the built assembly.  I didn't test Docker yet.




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org