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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
