[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1774: SOLR-14774: Create HealthCheckHandler in CoreContainer

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

tflobbe commented on a change in pull request #1774:
URL: https://github.com/apache/lucene-solr/pull/1774#discussion_r475756846



##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -719,6 +719,7 @@ public void load() {
     createHandler(ZK_PATH, ZookeeperInfoHandler.class.getName(), ZookeeperInfoHandler.class);
     createHandler(ZK_STATUS_PATH, ZookeeperStatusHandler.class.getName(), ZookeeperStatusHandler.class);
     collectionsHandler = createHandler(COLLECTIONS_HANDLER_PATH, cfg.getCollectionsHandlerClass(), CollectionsHandler.class);
+    healthCheckHandler = loader.newInstance(cfg.getHealthCheckHandlerClass(), HealthCheckHandler.class, null, new Class<?>[]{CoreContainer.class}, new Object[]{this});

Review comment:
       > why are we calling loader.newInstance directly instead of createHandler?
   
   I did this because `createHandler` also registers the handler to a path and initializes metrics. HealthCheckHandler currently is ran inside InfoHandler, so that's where the path is registered (and I guess the metrics are merged there). I didn't want to move to another path as part of this PR, don't know if that's needed. True that it looks a bit weird, but otherwise requires a bunch of changes to support compatibility.
   > If the intent here to use loader.newInstance is for pluggable implementations, then this may not work with packages. Hence, this should be avoided.
   
   I can handle that? I don't see any of the other top level handlers load from packages other than the built in (which is what `loader.newInstance`. What do you suggest?




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