Solr
  1. Solr
  2. SOLR-3301

Migrate enable/disable Ping from JSP to PingRequestHandler

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.0-ALPHA
    • Fix Version/s: 4.0-ALPHA
    • Component/s: web gui
    • Labels:
      None

      Description

      My colleague @omnifroodle and I noticed that you can't enable/disable pings in 4.0 because action.jsp doesn't run. This patch attaches the functionality to the PingRequestHandler. We debated creating a new HealthcheckRequestHandler as well. We added some basic unit tests.

      1. health_check_admin_consolidate.patch
        17 kB
        Matt Overstreet
      2. health_check_admin_consolidate2.patch
        17 kB
        Matt Overstreet
      3. healthcheckenable.patch
        8 kB
        Matt Overstreet
      4. healthcheck-ui.png
        66 kB
        Matt Overstreet
      5. ping_request_handler.patch
        3 kB
        Eric Pugh
      6. PingRequestHandlerTest.java
        3 kB
        Eric Pugh
      7. SOLR-3301.patch
        16 kB
        Stefan Matheis (steffkes)
      8. SOLR-3301-tests.patch
        0.8 kB
        Stefan Matheis (steffkes)

        Activity

        Hide
        Eric Pugh added a comment -

        Patch to PingRequestHandler

        Show
        Eric Pugh added a comment - Patch to PingRequestHandler
        Hide
        Eric Pugh added a comment -

        Supporting unit tests.

        Show
        Eric Pugh added a comment - Supporting unit tests.
        Hide
        Eric Pugh added a comment -

        Anyone have thoughts on where the disable/enable UI component should go?

        Show
        Eric Pugh added a comment - Anyone have thoughts on where the disable/enable UI component should go?
        Hide
        Matt Overstreet added a comment -

        example of new ui for health check

        Show
        Matt Overstreet added a comment - example of new ui for health check
        Hide
        Matt Overstreet added a comment -

        Patch for to include ui elements for healthcheck on the admin dashboard. Allows enabling and disabling of healthcheck through the admin.

        Show
        Matt Overstreet added a comment - Patch for to include ui elements for healthcheck on the admin dashboard. Allows enabling and disabling of healthcheck through the admin.
        Hide
        Matt Overstreet added a comment -

        Let me know if you have any thoughts on the UI elements I've added. I also refactored the admin css a bit to support using dd and dt with the same style as the statistics panel, but in other place in the admin.

        Show
        Matt Overstreet added a comment - Let me know if you have any thoughts on the UI elements I've added. I also refactored the admin css a bit to support using dd and dt with the same style as the statistics panel, but in other place in the admin.
        Hide
        Eric Pugh added a comment -

        Can this be committed?

        Show
        Eric Pugh added a comment - Can this be committed?
        Hide
        Stefan Matheis (steffkes) added a comment -

        Would you mind to create one (combined) Patch for me? Then i'd go ahead and commit this one

        Show
        Stefan Matheis (steffkes) added a comment - Would you mind to create one (combined) Patch for me? Then i'd go ahead and commit this one
        Hide
        Matt Overstreet added a comment -

        consolidated patch. includes all changes from the other attached patches as requested.

        Show
        Matt Overstreet added a comment - consolidated patch. includes all changes from the other attached patches as requested.
        Hide
        Stefan Matheis (steffkes) added a comment -

        Hm, i'm not completely happy with this one. if you're using it w/ the example-configuration, there is no healthcheck file defined. the ui will indicate a disabled status and if you try to enable it, it will fail - but w/o any visual notification?

        In addition to that, the Ping Handler is only displaying a status: "healthcheck not configured" information, while using a 200 OK Status, can we have something like a 503 Service Unavailable? To show that it's not "ready" at all?

        Show
        Stefan Matheis (steffkes) added a comment - Hm, i'm not completely happy with this one. if you're using it w/ the example-configuration, there is no healthcheck file defined. the ui will indicate a disabled status and if you try to enable it, it will fail - but w/o any visual notification? In addition to that, the Ping Handler is only displaying a status: "healthcheck not configured" information, while using a 200 OK Status, can we have something like a 503 Service Unavailable ? To show that it's not "ready" at all?
        Hide
        Matt Overstreet added a comment -

        Updated to address Stefan's great issue list. disabled healthcheck now returns with a 503 status.

        Also, healthcheck status display works correctly if the healthcheck file is commented out at server start (the entire control is grayed out).

        Let me know if I need to make any more tweaks!

        Show
        Matt Overstreet added a comment - Updated to address Stefan's great issue list. disabled healthcheck now returns with a 503 status. Also, healthcheck status display works correctly if the healthcheck file is commented out at server start (the entire control is grayed out). Let me know if I need to make any more tweaks!
        Hide
        Stefan Matheis (steffkes) added a comment -

        Thanks Matt, i've changed the following things:

        1) Lucene is using two spaces for indentation, i replaced the tabs.

        2) Your Patch did not compile:

        +    if (healthcheck == null) {
        +      throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE x, 
        +        "No healthcheck file defined.");
        +    }

        was failing with the following error(s):

        common.compile-core:
            [mkdir] Created dir: /opt/solr-trunk/solr/build/solr-core/classes/java
            [javac] Compiling 564 source files to /opt/solr-trunk/solr/build/solr-core/classes/java
            [javac] /opt/solr-trunk/solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java:141: ')' expected
            [javac]       throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE x, 
            [javac]                                                                          ^
            [javac] /opt/solr-trunk/solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java:141: illegal start of expression
            [javac]       throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE x, 
            [javac]                                                                            ^
            [javac] /opt/solr-trunk/solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java:141: ';' expected
            [javac]       throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE x, 
            [javac]                                                                             ^
            [javac] /opt/solr-trunk/solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java:142: illegal start of expression
            [javac]         "No healthcheck file defined.");
            [javac]                                       ^
            [javac] 4 errors

        3) I would not expect a BAD_REQUEST-Error if Ping is not configured?

        +      case STATUS:
        +        if( healthcheck == null){
        +          SolrException e = new SolrException(SolrException.ErrorCode.BAD_REQUEST, "healthcheck not configured");
        +          rsp.setException(e);
        +        }

        It's not the Clients Fault (which is, what the 4xx-Status-Range is meant for) - i changed this one into a SERVICE_UNAVAILABLE, which reflects the behavior more correct imho.

        Let me know if these are okay for you, then i'll go ahead and commit this one

        Show
        Stefan Matheis (steffkes) added a comment - Thanks Matt, i've changed the following things: 1) Lucene is using two spaces for indentation, i replaced the tabs. 2) Your Patch did not compile: + if (healthcheck == null ) { + throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE x, + "No healthcheck file defined." ); + } was failing with the following error(s): common.compile-core: [mkdir] Created dir: /opt/solr-trunk/solr/build/solr-core/classes/java [javac] Compiling 564 source files to /opt/solr-trunk/solr/build/solr-core/classes/java [javac] /opt/solr-trunk/solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java:141: ')' expected [javac] throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE x, [javac] ^ [javac] /opt/solr-trunk/solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java:141: illegal start of expression [javac] throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE x, [javac] ^ [javac] /opt/solr-trunk/solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java:141: ';' expected [javac] throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE x, [javac] ^ [javac] /opt/solr-trunk/solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java:142: illegal start of expression [javac] "No healthcheck file defined." ); [javac] ^ [javac] 4 errors 3) I would not expect a BAD_REQUEST -Error if Ping is not configured? + case STATUS: + if ( healthcheck == null ){ + SolrException e = new SolrException(SolrException.ErrorCode.BAD_REQUEST, "healthcheck not configured" ); + rsp.setException(e); + } It's not the Clients Fault (which is, what the 4xx-Status-Range is meant for) - i changed this one into a SERVICE_UNAVAILABLE , which reflects the behavior more correct imho. Let me know if these are okay for you, then i'll go ahead and commit this one
        Hide
        Matt Overstreet added a comment -

        Looks great,please go ahead.

        Show
        Matt Overstreet added a comment - Looks great,please go ahead.
        Hide
        Stefan Matheis (steffkes) added a comment -

        Committed in r1329263

        Show
        Stefan Matheis (steffkes) added a comment - Committed in r1329263
        Show
        Stefan Matheis (steffkes) added a comment - https://builds.apache.org/job/Lucene-Solr-tests-only-trunk/13442/testReport/junit/org.apache.solr.handler/PingRequestHandlerTest/testPing/ is failing
        Hide
        Chris Male added a comment -

        As Stefan found out, the problem was that a locale wasn't specified in the toUpperCase() call.

        Show
        Chris Male added a comment - As Stefan found out, the problem was that a locale wasn't specified in the toUpperCase() call.

          People

          • Assignee:
            Stefan Matheis (steffkes)
            Reporter:
            Eric Pugh
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development