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

      Description

      • make NamedListCodec a generic class which may be able to write any of the supported object
      • Deprecate NamedList and use only SimpleOrderedMap. (Yes it is backward compatible)
      • Ideally I wish to call this JavabinCodec because it should be able to be used to write any kind of supported objects
      1. SOLR-885.patch
        7 kB
        Shalin Shekhar Mangar
      2. SOLR-885.patch
        6 kB
        Shalin Shekhar Mangar
      3. SOLR-885.patch
        49 kB
        Noble Paul
      4. SOLR-885.patch
        7 kB
        Noble Paul
      5. SOLR-885.patch
        7 kB
        Noble Paul

        Activity

        Hide
        Noble Paul added a comment -

        this also throws an exception for invalid version

        Show
        Noble Paul added a comment - this also throws an exception for invalid version
        Hide
        Yonik Seeley added a comment -

        NamedList and SimpleOrderedMap have different semantics for JSON output (that's why there were two). It seems useful to be able to preserve that extra info in javabin since the final client request may be in JSON.

        Show
        Yonik Seeley added a comment - NamedList and SimpleOrderedMap have different semantics for JSON output (that's why there were two). It seems useful to be able to preserve that extra info in javabin since the final client request may be in JSON.
        Hide
        Noble Paul added a comment -

        Patch w/o deprecating NamedList .

        But , before javabin we used to use xml which has same Semantics for namedList and SimpleOrderedMap it is right?

        Show
        Noble Paul added a comment - Patch w/o deprecating NamedList . But , before javabin we used to use xml which has same Semantics for namedList and SimpleOrderedMap it is right?
        Hide
        Ryan McKinley added a comment -

        But , before javabin we used to use xml which has same Semantics for namedList and SimpleOrderedMap it is right?

        You are talking about solrj, correct? Yes - by default we used a syntax that keeps order. But that does not mean the difference between the classes is not useful for other cases

        Ideally the javabin implementation would know if the result was originally SimpleOrderedMap or not. This would only really be important in the case where you use javabin to pass on a result to a client (distributed?)

        Show
        Ryan McKinley added a comment - But , before javabin we used to use xml which has same Semantics for namedList and SimpleOrderedMap it is right? You are talking about solrj, correct? Yes - by default we used a syntax that keeps order. But that does not mean the difference between the classes is not useful for other cases Ideally the javabin implementation would know if the result was originally SimpleOrderedMap or not. This would only really be important in the case where you use javabin to pass on a result to a client (distributed?)
        Hide
        Noble Paul added a comment -

        by default we used a syntax that keeps order. But that does not mean the difference between the classes is not useful for other cases

        I do not see any difference between NamedList and SimpleOrderedMap . Both has same semantics . So, having different representation in serialized formats look odd

        Show
        Noble Paul added a comment - by default we used a syntax that keeps order. But that does not mean the difference between the classes is not useful for other cases I do not see any difference between NamedList and SimpleOrderedMap . Both has same semantics . So, having different representation in serialized formats look odd
        Hide
        Yonik Seeley added a comment -

        I do not see any difference between NamedList and SimpleOrderedMap . Both has same semantics.

        See the javadoc for SimpleOrderedMap:

        /** <code>SimpleOrderedMap</code> is a {@link NamedList} where access by key is more
         * important than maintaining order when it comes to representing the
         * held data in other forms, as ResponseWriters normally do.
         * It's normally not a good idea to repeat keys or use null keys, but this
         * is not enforced.  If key uniqueness enforcement is desired, use a regular {@link Map}.
         * <p>
         * For example, a JSON response writer may choose to write a SimpleOrderedMap
         * as {"foo":10,"bar":20} and may choose to write a NamedList as
         * ["foo",10,"bar",20].  An XML response writer may choose to render both
         * the same way.
         * </p>
        

        The problem is that some clients don't maintain order (for example, doing eval() of JSON/python/ruby output). It wouldn't be an issue if we had control over all clients, but we don't.

        Show
        Yonik Seeley added a comment - I do not see any difference between NamedList and SimpleOrderedMap . Both has same semantics. See the javadoc for SimpleOrderedMap: /** <code>SimpleOrderedMap</code> is a {@link NamedList} where access by key is more * important than maintaining order when it comes to representing the * held data in other forms, as ResponseWriters normally do . * It's normally not a good idea to repeat keys or use null keys, but this * is not enforced. If key uniqueness enforcement is desired, use a regular {@link Map}. * <p> * For example, a JSON response writer may choose to write a SimpleOrderedMap * as { "foo" :10, "bar" :20} and may choose to write a NamedList as * [ "foo" ,10, "bar" ,20]. An XML response writer may choose to render both * the same way. * </p> The problem is that some clients don't maintain order (for example, doing eval() of JSON/python/ruby output). It wouldn't be an issue if we had control over all clients, but we don't.
        Hide
        Noble Paul added a comment -

        OK. let us not make the change and stick to the original description of the bug make NamedListCodec generic
        the latest patch just does that

        Show
        Noble Paul added a comment - OK. let us not make the change and stick to the original description of the bug make NamedListCodec generic the latest patch just does that
        Hide
        Shalin Shekhar Mangar added a comment -

        I think we can make this one change. No need to deprecate NamedList and use only SimpleOrderedMap.

        Are there any concerns?

        Show
        Shalin Shekhar Mangar added a comment - I think we can make this one change. No need to deprecate NamedList and use only SimpleOrderedMap. Are there any concerns?
        Hide
        Yonik Seeley added a comment -

        Noble, did you attach the latest patch?

        Show
        Yonik Seeley added a comment - Noble, did you attach the latest patch?
        Hide
        Noble Paul added a comment -

        after Ryan moved the packages around the patch became invalid

        new patch to sync with trunk

        some fields/methods are changed from private to public/protected so that this can be used extend the format

        Show
        Noble Paul added a comment - after Ryan moved the packages around the patch became invalid new patch to sync with trunk some fields/methods are changed from private to public/protected so that this can be used extend the format
        Hide
        Noble Paul added a comment - - edited

        NamedListCodec is renamed as JavabinCodec over and above the previous patch

        NamedListCodec now extends JavabinCodec and it is also marked as deprected

        Show
        Noble Paul added a comment - - edited NamedListCodec is renamed as JavabinCodec over and above the previous patch NamedListCodec now extends JavabinCodec and it is also marked as deprected
        Hide
        Shalin Shekhar Mangar added a comment -

        This looks good Noble. Let us do the rename as another patch/commit. It is difficult to figure out the changes with the renamed source file. We also need to update the Javadocs.

        Show
        Shalin Shekhar Mangar added a comment - This looks good Noble. Let us do the rename as another patch/commit. It is difficult to figure out the changes with the renamed source file. We also need to update the Javadocs.
        Hide
        Noble Paul added a comment -

        My second last patch and the latest patch has only this one difference (the rename)

        Show
        Noble Paul added a comment - My second last patch and the latest patch has only this one difference (the rename)
        Hide
        Yonik Seeley added a comment -

        What is gained by making NamedListCodec a generic?

        Show
        Yonik Seeley added a comment - What is gained by making NamedListCodec a generic?
        Hide
        Noble Paul added a comment -

        NamedListCodec dictates the root Object to be a NamedList . It is useful if we can write other supported types as the root object. say Map, List

        Why give a special status to NamedList when the implementation has no such constraints?

        Show
        Noble Paul added a comment - NamedListCodec dictates the root Object to be a NamedList . It is useful if we can write other supported types as the root object. say Map, List Why give a special status to NamedList when the implementation has no such constraints?
        Hide
        Yonik Seeley added a comment -

        NamedListCodec dictates the root Object to be a NamedList

        Agreed, but theType parameter is only used to cast the final root Object to the Type.

        So instead of this big patch, simply change the signature of the method:
        public NamedList unmarshal(InputStream is)
        To
        public Object unmarshal(InputStream is)

        Show
        Yonik Seeley added a comment - NamedListCodec dictates the root Object to be a NamedList Agreed, but theType parameter is only used to cast the final root Object to the Type. So instead of this big patch, simply change the signature of the method: public NamedList unmarshal(InputStream is) To public Object unmarshal(InputStream is)
        Hide
        Noble Paul added a comment -

        So instead of this big patch, simply change the signature of the method:

        public NamedList unmarshal(InputStream is)

        To

        public Object unmarshal(InputStream is)

        This is fine .No need to make it generic

        But renaming the class to JavabinCodec makes sense right?

        Show
        Noble Paul added a comment - So instead of this big patch, simply change the signature of the method: public NamedList unmarshal(InputStream is) To public Object unmarshal(InputStream is) This is fine .No need to make it generic But renaming the class to JavabinCodec makes sense right?
        Hide
        Yonik Seeley added a comment -

        But renaming the class to JavabinCodec makes sense right?

        Yep.

        Of course, I was never quite sure why it was "javabin" and not just "bin" or "binary".

        Show
        Yonik Seeley added a comment - But renaming the class to JavabinCodec makes sense right? Yep. Of course, I was never quite sure why it was "javabin" and not just "bin" or "binary".
        Hide
        Noble Paul added a comment -

        Of course, I was never quite sure why it was "javabin" and not just "bin" or "binary".

        Frankly, I don't have an answer. It just happened.
        My thought process was that there would be other binary formats coming up later protobuf/thrift etc.

        Show
        Noble Paul added a comment - Of course, I was never quite sure why it was "javabin" and not just "bin" or "binary". Frankly, I don't have an answer. It just happened. My thought process was that there would be other binary formats coming up later protobuf/thrift etc.
        Hide
        Shalin Shekhar Mangar added a comment -

        With this patch NamedListCodec returns Object instead.

        Show
        Shalin Shekhar Mangar added a comment - With this patch NamedListCodec returns Object instead.
        Hide
        Shalin Shekhar Mangar added a comment -

        Forgot to change TestBinaryResponseWriter for the change in NamedListCodec

        Show
        Shalin Shekhar Mangar added a comment - Forgot to change TestBinaryResponseWriter for the change in NamedListCodec
        Hide
        Shalin Shekhar Mangar added a comment -

        Committed revision 728017.

        I'll give a patch now to rename NamedListCodec to JavaBinCodec.

        Show
        Shalin Shekhar Mangar added a comment - Committed revision 728017. I'll give a patch now to rename NamedListCodec to JavaBinCodec.
        Hide
        Shalin Shekhar Mangar added a comment -

        Committed revision 728622.

        Show
        Shalin Shekhar Mangar added a comment - Committed revision 728622.
        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:
            Shalin Shekhar Mangar
            Reporter:
            Noble Paul
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development