Solr
  1. Solr
  2. SOLR-967

NamedList - Deprecating ctor. with heterogenous List and replacing with a type-safe variant.

    Details

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

      Java 6, Tomcat 6

      Description

      NamedList (org/apache/solr/common/util/NamedList ) currently has a heterogenous List (even numbered indices are String-s ) and the odd-numbered indices are the specific types. As per benchmarks ( see SOLR-912 ) - the implementation could be improved in favor of a Map.Entry<String, T> that beats in performance and ease of code maintenance.

      As per the discussion in SOLR-912 , a separate issue in JIRA is created that temporarily deprecates the List ctor. in NamedList and replaces the same with a Map.Entry<String, T> [] arg. constructor .

      This would be go in 1.4 to enable people to migrate their code for the new ctor. and move away from List<?> .

      At a later version of Solr (may be v2) - the optimal implementation would be brought back in with the List <Map.Entry<String, ?> > for better type-safety and performance.

      1. SOLR-967.patch
        10 kB
        Hoss Man
      2. SOLR-967.patch
        10 kB
        Karthik K

        Issue Links

          Activity

          Hide
          Karthik K added a comment -
          1. add an "Entry<String, T>[]" constructor (with a warning, but no commitment, that modifying the array contents may affect the NamedList) which builds up a pairwise List<Object> and delegates to the existing "List" constructor

          Once we build a pairwise List<Object> and delegate to the external "List" ctor , I am not sure how we can give a 'partial' guarantee that modifying the array affecting NamedList since we are essentially flattening the former data structure (Map.Entry<String, T> ) into the latter ( String key; T value; }

          So - I have added documentation such a way that the guarantee is not possible.

          2. deprecate the existing List constructor.

          In addition to deprecating the existing ctor. the comment has also been modified such that the external modification guarantee would not be held again in the future implementations and users are advised to use add / remove etc. for the modification as opposed to changing the data structure without the knowledge of the class.

          In fact- if we eventually bring back List < Map.Entry<String, ? > > , the external modification guarantee might be honored although I am not sure if that is a big development plus as providing multiple ways to modify the underlying data structure might be hard to track in the code.

          Let me know if this patch would provide the intended transition.

          Show
          Karthik K added a comment - 1. add an "Entry<String, T>[]" constructor (with a warning, but no commitment, that modifying the array contents may affect the NamedList) which builds up a pairwise List<Object> and delegates to the existing "List" constructor Once we build a pairwise List<Object> and delegate to the external "List" ctor , I am not sure how we can give a 'partial' guarantee that modifying the array affecting NamedList since we are essentially flattening the former data structure (Map.Entry<String, T> ) into the latter ( String key; T value; } So - I have added documentation such a way that the guarantee is not possible. 2. deprecate the existing List constructor. In addition to deprecating the existing ctor. the comment has also been modified such that the external modification guarantee would not be held again in the future implementations and users are advised to use add / remove etc. for the modification as opposed to changing the data structure without the knowledge of the class. In fact- if we eventually bring back List < Map.Entry<String, ? > > , the external modification guarantee might be honored although I am not sure if that is a big development plus as providing multiple ways to modify the underlying data structure might be hard to track in the code. Let me know if this patch would provide the intended transition.
          Hide
          Hoss Man added a comment -

          Kay: I made a few modifications to your patch...

          1) tweaked some of the docs. you had a cut/paste sentence in the new constructor that contradicted the later sentences, i also cleaned up exactly what the commitments are from each constructor, and what the expectations should be for users. We also don't need to tell people that the explicit backing of the LIst constructor will go away in future versions, because it will only go away when the method goes away (we can't add a pure List<Entry> constructor for a while after removing the current List constructor because of type erasure and people possibly skipping versions when upgrading)

          2) i changed the new constructor to take Map.Entry<String, ? extends T>[] instead of Map.Entry<String, ?>[] ... unless i'm missing something a completely unbounded type wildcard would be just as bad as the untyped List.

          how does this look to you?

          Show
          Hoss Man added a comment - Kay: I made a few modifications to your patch... 1) tweaked some of the docs. you had a cut/paste sentence in the new constructor that contradicted the later sentences, i also cleaned up exactly what the commitments are from each constructor, and what the expectations should be for users. We also don't need to tell people that the explicit backing of the LIst constructor will go away in future versions, because it will only go away when the method goes away (we can't add a pure List<Entry> constructor for a while after removing the current List constructor because of type erasure and people possibly skipping versions when upgrading) 2) i changed the new constructor to take Map.Entry<String, ? extends T>[] instead of Map.Entry<String, ?>[] ... unless i'm missing something a completely unbounded type wildcard would be just as bad as the untyped List. how does this look to you?
          Hide
          Karthik K added a comment -

          1) Thanks for helping with the docs.

          2) ? extends T - makes perfect sense since it provides one more degree of flexibility while preserving the contract about the type T .

          Show
          Karthik K added a comment - 1) Thanks for helping with the docs. 2) ? extends T - makes perfect sense since it provides one more degree of flexibility while preserving the contract about the type T .
          Hide
          Karthik K added a comment -

          The revised patch looks ok to me.

          Show
          Karthik K added a comment - The revised patch looks ok to me.
          Hide
          Hoss Man added a comment -

          Committed revision 746031.

          Show
          Hoss Man added a comment - Committed revision 746031.
          Hide
          Grant Ingersoll added a comment -

          Bulk close for Solr 1.4

          Show
          Grant Ingersoll added a comment - Bulk close for Solr 1.4

            People

            • Assignee:
              Hoss Man
              Reporter:
              Karthik K
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

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

                  Development