Solr
  1. Solr
  2. SOLR-8648

Support selective clearing up of stored async collection API responses

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      The only way to clear up stored collection API responses right now is by sending in '-1' as the request id in the REQUESTSTATUS call. It makes a lot of sense to support selective deletion of stored responses so the ids could be reused.

      1. SOLR-8648.patch
        23 kB
        Anshum Gupta
      2. SOLR-8648.patch
        26 kB
        Anshum Gupta
      3. SOLR-8648.patch
        21 kB
        Anshum Gupta
      4. SOLR-8648.patch
        10 kB
        Anshum Gupta

        Activity

        Hide
        Anshum Gupta added a comment -

        Patch with test

        Show
        Anshum Gupta added a comment - Patch with test
        Hide
        Shai Erera added a comment -

        if(req.getParams().getBool("cleanup")) {

        • Can you add a space after the 'if'?
        • Separately, would be nice if DistributedMap#remove() would return a boolean/Object, you can avoid the .contains followed by .remove check (which also means two accesses to ZK).

        .setCleanup(true).process(cloudClient);

        If you're already doing the builder style (which I like), mind as well write that as:

          .setCleanup(true)
          .process(cloudClient);
        

        Some general comments:

        • Can you also add a test which attempts to delete a non-existing requestId, as well one which attempts to delete the same twice?
        • How would the REST call look like? /solr/admin/collections?action=REQUESTSTATUS&requestId=foo&cleanup=true? I thought that we want to add a DELETESTATUS specific action, in which case you won't need to add a cleanup parameter?
        • What about &requestId=-1&cleanup=false? I mean, we obviously don't need to support all options, but was just wondering what would the code do? If we added a DELETESTATUS action, then we could deprecate requestId=-1 at all and remove in 6.0, in exchange for only the latter action. Also, V2-API would be able to handle that with a DELETE /solr/admin/... vs GET /solr/admin/....

        What do you think?

        Show
        Shai Erera added a comment - if(req.getParams().getBool("cleanup")) { Can you add a space after the 'if'? Separately, would be nice if DistributedMap#remove() would return a boolean/Object, you can avoid the .contains followed by .remove check (which also means two accesses to ZK). .setCleanup(true).process(cloudClient); If you're already doing the builder style (which I like), mind as well write that as: .setCleanup( true ) .process(cloudClient); Some general comments: Can you also add a test which attempts to delete a non-existing requestId, as well one which attempts to delete the same twice? How would the REST call look like? /solr/admin/collections?action=REQUESTSTATUS&requestId=foo&cleanup=true ? I thought that we want to add a DELETESTATUS specific action, in which case you won't need to add a cleanup parameter? What about &requestId=-1&cleanup=false ? I mean, we obviously don't need to support all options, but was just wondering what would the code do? If we added a DELETESTATUS action, then we could deprecate requestId=-1 at all and remove in 6.0, in exchange for only the latter action. Also, V2-API would be able to handle that with a DELETE /solr/admin/... vs GET /solr/admin/... . What do you think?
        Hide
        Anshum Gupta added a comment - - edited

        Thanks for the review Shai. Here's an updated patch that addresses your feedback. I have added a new Collections Admin API (DELETESTATUS) that accepts 2 parameters:

        • requestid (String)
        • flush (Boolean)

        This patch also has more tests including deleting the same response more than once and attempting to delete a non-existent response.

        Show
        Anshum Gupta added a comment - - edited Thanks for the review Shai. Here's an updated patch that addresses your feedback. I have added a new Collections Admin API (DELETESTATUS) that accepts 2 parameters: requestid (String) flush (Boolean) This patch also has more tests including deleting the same response more than once and attempting to delete a non-existent response.
        Hide
        Shai Erera added a comment - - edited

        Thanks Anshum Gupta, I've got few minor (style) comments:

        OverseerTaskProcessor

        • if(asyncId != null): can you add a space after the 'if'?
        • Someplace else in the file: if(!runningMap.remove(asyncId)) - also missing space

        DistributedMap.remove()

        • I believe the javadocs won't render well as the @return tag swallows newlines. You can either use an <ul> element, or document it in text instead of the tag.

        CollectionsHandler

        • public static final String FLUSH = "flush"; is also defined in CollectionAdminParams. Can we have just one definition?
        • I don't see a deprecation/removal of requestId == -1. Do you intend to do that separately?
        • if (requestId != null && flush) "Both requestid or flush" should be "Both requestid and flush"
        • if (req.getParams().getBool(FLUSH, false)) is redundant since you already set it to flush above
        • zkController.getOverseerFailureMap().remove(requestId); that's a mistake? You already call remove inside the 'if'

        DeleteStatusTest

        • // Try deleting the same response again s/response/requestid/ ?

        CoreAdminParams

        • public static final String CLEANUP = "cleanup" I didn't get where it's used in this patch?
        Show
        Shai Erera added a comment - - edited Thanks Anshum Gupta , I've got few minor (style) comments: OverseerTaskProcessor if(asyncId != null) : can you add a space after the 'if'? Someplace else in the file: if(!runningMap.remove(asyncId)) - also missing space DistributedMap.remove() I believe the javadocs won't render well as the @return tag swallows newlines. You can either use an <ul> element, or document it in text instead of the tag. CollectionsHandler public static final String FLUSH = "flush"; is also defined in CollectionAdminParams. Can we have just one definition? I don't see a deprecation/removal of requestId == -1 . Do you intend to do that separately? if (requestId != null && flush) "Both requestid or flush" should be "Both requestid and flush" if (req.getParams().getBool(FLUSH, false)) is redundant since you already set it to flush above zkController.getOverseerFailureMap().remove(requestId); that's a mistake? You already call remove inside the 'if' DeleteStatusTest // Try deleting the same response again s/response/requestid/ ? CoreAdminParams public static final String CLEANUP = "cleanup" I didn't get where it's used in this patch?
        Hide
        Anshum Gupta added a comment -

        Updated patch. Seems like my idea settings file is acting up. Hopefully the formatting is maintained this time around.

        Show
        Anshum Gupta added a comment - Updated patch. Seems like my idea settings file is acting up. Hopefully the formatting is maintained this time around.
        Hide
        Anshum Gupta added a comment -

        the previous patch had some rebase issues, here's another clean one.

        Show
        Anshum Gupta added a comment - the previous patch had some rebase issues, here's another clean one.
        Hide
        Shai Erera added a comment -

        Looks good Anshum Gupta! +1

        Show
        Shai Erera added a comment - Looks good Anshum Gupta ! +1
        Hide
        Anshum Gupta added a comment -

        Also, about the deprecation, I'll change the 5x commit to actually return a warning with the response body and also document it in the code.

        Show
        Anshum Gupta added a comment - Also, about the deprecation, I'll change the 5x commit to actually return a warning with the response body and also document it in the code.
        Hide
        ASF subversion and git services added a comment -

        Commit 03d7f80b27031309f7156af3bafcb6ccea74f7c7 in lucene-solr's branch refs/heads/master from anshum
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=03d7f80 ]

        SOLR-8648: Support selective clearing up of stored async collection API responses via DELETESTATUS API

        Show
        ASF subversion and git services added a comment - Commit 03d7f80b27031309f7156af3bafcb6ccea74f7c7 in lucene-solr's branch refs/heads/master from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=03d7f80 ] SOLR-8648 : Support selective clearing up of stored async collection API responses via DELETESTATUS API
        Hide
        ASF subversion and git services added a comment -

        Commit 3892f7e7a3ef41ce335f5cbfedc62e74489b0dd4 in lucene-solr's branch refs/heads/branch_5x from anshum
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3892f7e ]

        SOLR-8648: Support selective clearing up of stored async collection API responses via DELETESTATUS API

        Show
        ASF subversion and git services added a comment - Commit 3892f7e7a3ef41ce335f5cbfedc62e74489b0dd4 in lucene-solr's branch refs/heads/branch_5x from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3892f7e ] SOLR-8648 : Support selective clearing up of stored async collection API responses via DELETESTATUS API
        Hide
        Varun Thacker added a comment -

        Hi Anshum,

        In 6.x + we no longer support this {[action=REQUESTSTATUS&requestid=-1}} right?

        I saw two references of that usage https://cwiki.apache.org/confluence/display/solr/Collections+API#CollectionsAPI-RequestStatus and https://cwiki.apache.org/confluence/display/solr/Collections+API#CollectionsAPI-AsynchronousCalls . We can safely remove that right?

        Show
        Varun Thacker added a comment - Hi Anshum, In 6.x + we no longer support this {[action=REQUESTSTATUS&requestid=-1}} right? I saw two references of that usage https://cwiki.apache.org/confluence/display/solr/Collections+API#CollectionsAPI-RequestStatus and https://cwiki.apache.org/confluence/display/solr/Collections+API#CollectionsAPI-AsynchronousCalls . We can safely remove that right?
        Hide
        Anshum Gupta added a comment -

        Varun Thacker yes. I've removed this from both the places.

        Show
        Anshum Gupta added a comment - Varun Thacker yes. I've removed this from both the places.

          People

          • Assignee:
            Anshum Gupta
            Reporter:
            Anshum Gupta
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development