Solr
  1. Solr
  2. SOLR-916

CoreContainer :: register(String, SolrCore, boolean) documentation clarification about returnPrev argument

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3
    • Fix Version/s: 1.4
    • Component/s: None
    • Labels:
      None
    • Environment:

      Tomcat 6, JRE 6

      Description

      In CoreContainer.java :: register(name, core, returnPrev) - the documentation says
      it would return a previous core having the same name if it existed and returnPrev = true.

      • @return a previous core having the same name if it existed and returnPrev==true
        */
        public SolrCore register(String name, SolrCore core, boolean returnPrev) ..

      But as per the code towards the end - the previous core is returned anyway, irrespective of the value of returnPrev. The difference, though, seems to be that when returnPrev is false, the previous core (of the same name, if exists) is closed.

      Which one of them is correct . If the code were correct , would the variable be better renamed as closePrevious , as opposed to returnPrevious.

        Activity

        Hide
        Grant Ingersoll added a comment -

        Bulk close for Solr 1.4

        Show
        Grant Ingersoll added a comment - Bulk close for Solr 1.4
        Hide
        Noble Paul added a comment - - edited

        the parameter is renamed . I hope it is fine now
        committed r790041

        Show
        Noble Paul added a comment - - edited the parameter is renamed . I hope it is fine now committed r790041
        Hide
        Karthik K added a comment - - edited
        I guess we can just document the behavior and keep the variable name as is. If it is ok I shall just make the changes and close this issue

        I disagree. The name of the variable does not reflect the actual underlying behavior . We need the documentation change as well as an appropriate name of the variable fitting the current behavior.

        Show
        Karthik K added a comment - - edited I guess we can just document the behavior and keep the variable name as is. If it is ok I shall just make the changes and close this issue I disagree. The name of the variable does not reflect the actual underlying behavior . We need the documentation change as well as an appropriate name of the variable fitting the current behavior.
        Hide
        Noble Paul added a comment -

        I guess we can just document the behavior and keep the variable name as is. If it is ok I shall just make the changes and close this issue

        Show
        Noble Paul added a comment - I guess we can just document the behavior and keep the variable name as is. If it is ok I shall just make the changes and close this issue
        Hide
        Mark Miller added a comment -

        I would say that if returnPrev is false, what is returned is undefined. The closed core is returned, but it just as well could be null. If the user expects to get and be able to use the prevCore, he must pass returnPrev = true.

        It still makes sense to me, but I wouldn't object to the change if someone wants to put it in.

        Show
        Mark Miller added a comment - I would say that if returnPrev is false, what is returned is undefined. The closed core is returned, but it just as well could be null. If the user expects to get and be able to use the prevCore, he must pass returnPrev = true. It still makes sense to me, but I wouldn't object to the change if someone wants to put it in.
        Hide
        Karthik K added a comment -
        I could go either way. The name change certainly makes some sense, but even as is, a core that is returned closed is not very useful. Null could just as well be returned. But when you ask to returnPrev, a working open core is guaranteed to be returned if a previous one existed. I wouldnt feel so bad leaving it as it is - your change almost seems to strengthen the contract - you have to return the closed core as well. Neither side is a very strong argument to me and I could go either way.

        I do not have an issue with the logic ( closing an existing core if returnPrev is false ) as it is currently but am more concerned with the name of the variable and the documentation about the same - just to make sure that it is in sync with the actual logic as of today.

        Show
        Karthik K added a comment - I could go either way. The name change certainly makes some sense, but even as is, a core that is returned closed is not very useful. Null could just as well be returned. But when you ask to returnPrev, a working open core is guaranteed to be returned if a previous one existed. I wouldnt feel so bad leaving it as it is - your change almost seems to strengthen the contract - you have to return the closed core as well. Neither side is a very strong argument to me and I could go either way. I do not have an issue with the logic ( closing an existing core if returnPrev is false ) as it is currently but am more concerned with the name of the variable and the documentation about the same - just to make sure that it is in sync with the actual logic as of today.
        Hide
        Mark Miller added a comment -

        I could go either way. The name change certainly makes some sense, but even as is, a core that is returned closed is not very useful. Null could just as well be returned. But when you ask to returnPrev, a working open core is guaranteed to be returned if a previous one existed. I wouldnt feel so bad leaving it as it is - your change almost seems to strengthen the contract - you have to return the closed core as well. Neither side is a very strong argument to me and I could go either way.

        Show
        Mark Miller added a comment - I could go either way. The name change certainly makes some sense, but even as is, a core that is returned closed is not very useful. Null could just as well be returned. But when you ask to returnPrev, a working open core is guaranteed to be returned if a previous one existed. I wouldnt feel so bad leaving it as it is - your change almost seems to strengthen the contract - you have to return the closed core as well. Neither side is a very strong argument to me and I could go either way.
        Hide
        Karthik K added a comment -

        appropriate rename of closePrevious to donotClosePrevious .

        Show
        Karthik K added a comment - appropriate rename of closePrevious to donotClosePrevious .
        Hide
        Karthik K added a comment -

        returnPrevious boolean renamed to closePrevious.

        Javadoc modified to reflect exactly what is happening in the code.

        Show
        Karthik K added a comment - returnPrevious boolean renamed to closePrevious. Javadoc modified to reflect exactly what is happening in the code.
        Hide
        Karthik K added a comment -

        Renaming the 3rd field to be closeprevious and fixing documentation to reflect the code appropriately.

        Show
        Karthik K added a comment - Renaming the 3rd field to be closeprevious and fixing documentation to reflect the code appropriately.
        Hide
        Karthik K added a comment -

        Can somebody familiar with this part - confirm / add more light on what the third argument supposedly is intended to do (as compared to what it is doing today ).

        Show
        Karthik K added a comment - Can somebody familiar with this part - confirm / add more light on what the third argument supposedly is intended to do (as compared to what it is doing today ).
        Hide
        Karthik K added a comment -

        Patch attached . More documentation added for the arguments. Instead of RuntimeException - IllegalArgumentException is thrown to be more specific about the error. (backward compatible since IRE inherits from RTE too).

        Show
        Karthik K added a comment - Patch attached . More documentation added for the arguments. Instead of RuntimeException - IllegalArgumentException is thrown to be more specific about the error. (backward compatible since IRE inherits from RTE too).

          People

          • Assignee:
            Noble Paul
            Reporter:
            Karthik K
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 2h
              2h
              Remaining:
              Remaining Estimate - 2h
              2h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development