You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/08/22 03:57:14 UTC
[GitHub] [lucene-solr] tflobbe opened a new pull request #1774: SOLR-14774: Create HealthCheckHandler in CoreContainer
tflobbe opened a new pull request #1774:
URL: https://github.com/apache/lucene-solr/pull/1774
This allows users to plug-in different implementations of the handler (they must extend HealthCheckHandler)
This PR also removes the HealthCheckHandler from the implicit SolrCore plugins
----------------------------------------------------------------
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
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1774: SOLR-14774: Create HealthCheckHandler in CoreContainer
Posted by GitBox <gi...@apache.org>.
tflobbe commented on a change in pull request #1774:
URL: https://github.com/apache/lucene-solr/pull/1774#discussion_r477616371
##########
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:
> As far as I can tell InfoHandler does not do any metric initialisation,
From what I can see, `InfoHandler` extends `SolrMetricProducer`, so a call to `CoreContainer.createHandler` will initialize it (seems to be getting everything from`RequestHandlerBase`
bq. I wonder if it would be helpful to have that as a comment in the code for future reference, the bit about not wishing to register to a path and initialise metrics.
Will do
----------------------------------------------------------------
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
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1774: SOLR-14774: Create HealthCheckHandler in CoreContainer
Posted by 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
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1774: SOLR-14774: Create HealthCheckHandler in CoreContainer
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1774:
URL: https://github.com/apache/lucene-solr/pull/1774#discussion_r475556783
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/InfoHandler.java
##########
@@ -50,7 +50,10 @@ public InfoHandler(final CoreContainer coreContainer) {
handlers.put("properties", new PropertiesRequestHandler());
handlers.put("logging", new LoggingHandler(coreContainer));
handlers.put("system", new SystemInfoHandler(coreContainer));
- handlers.put("health", new HealthCheckHandler(coreContainer));
+ if (coreContainer.getHealthCheckHandler() == null) {
+ throw new IllegalStateException("HealthCheckHandler needs to be initialized before creating InfoHandler");
Review comment:
> `throw new IllegalStateException("HealthCheckHandler needs to be initialized before creating InfoHandler");`
How about also having a "HealthCheckHandler then InfoHandler order matters because ..." style comment in `CoreContainer`?
----------------------------------------------------------------
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
[GitHub] [lucene-solr] madrob commented on a change in pull request #1774: SOLR-14774: Create HealthCheckHandler in CoreContainer
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #1774:
URL: https://github.com/apache/lucene-solr/pull/1774#discussion_r475118313
##########
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?
----------------------------------------------------------------
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
[GitHub] [lucene-solr] tflobbe merged pull request #1774: SOLR-14774: Create HealthCheckHandler in CoreContainer
Posted by GitBox <gi...@apache.org>.
tflobbe merged pull request #1774:
URL: https://github.com/apache/lucene-solr/pull/1774
----------------------------------------------------------------
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
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1774: SOLR-14774: Create HealthCheckHandler in CoreContainer
Posted by GitBox <gi...@apache.org>.
tflobbe commented on a change in pull request #1774:
URL: https://github.com/apache/lucene-solr/pull/1774#discussion_r475802739
##########
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:
@chatman, looking a bit at the Packages code. Is the idea that we need to use `PackageListeningClassLoader`? That's not currently used in CoreContainer, and I only see it for loading schema plugins. Maybe I'm missing something. If none of the existing top level handlers can be loaded from packages, then we probably want to tackle that in a different Jira issue?
----------------------------------------------------------------
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
[GitHub] [lucene-solr] chatman commented on a change in pull request #1774: SOLR-14774: Create HealthCheckHandler in CoreContainer
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1774:
URL: https://github.com/apache/lucene-solr/pull/1774#discussion_r475624794
##########
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:
If the intent here to use loader.newInstance is for pluggable implementations, then this may not work with packages. Hence, this should be avoided.
----------------------------------------------------------------
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
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1774: SOLR-14774: Create HealthCheckHandler in CoreContainer
Posted by 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
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1774: SOLR-14774: Create HealthCheckHandler in CoreContainer
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1774:
URL: https://github.com/apache/lucene-solr/pull/1774#discussion_r477438969
##########
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:
> ... 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 wonder if it would be helpful to have that as a comment in the code for future reference, the bit about not wishing to register to a path and initialise metrics.
As far as I can tell `InfoHandler` does not do any metric initialisation, so the health check handler would then be consistent with the other handlers whose path is registered in `InfoHandler`. Or maybe I'm missing how the metrics initialisation happens indirectly?
----------------------------------------------------------------
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