Solr
  1. Solr
  2. SOLR-969

Context#FULL_DUMP, DELTA_DUMP, FIND_DELTA as an enum

    Details

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

      Java 6, Tomcat 6

      Description

      Context # FULL_DUMP, DELTA_DUMP, FIND_DELTA made type-safe as an enum.

      1. SOLR-969.patch
        30 kB
        Shalin Shekhar Mangar
      2. SOLR-969.patch
        28 kB
        Shalin Shekhar Mangar
      3. SOLR-969.patch
        28 kB
        Karthik K

        Activity

        Hide
        Karthik K added a comment -

        New enum value called IMPORT_INVALID for corner cases.

        Show
        Karthik K added a comment - New enum value called IMPORT_INVALID for corner cases.
        Hide
        Noble Paul added a comment -

        It was not made an enum because I wamted them to be used in non-java languages (like javascript)

        Show
        Noble Paul added a comment - It was not made an enum because I wamted them to be used in non-java languages (like javascript)
        Hide
        Karthik K added a comment -

        Are there any unit test cases for the scenario that you are mentioning about. Also Context has EntityProcessor / DataSource / VariableResolver as return types .What would be the right way to serialize the Java objects to the JavaSCript realm .

        Show
        Karthik K added a comment - Are there any unit test cases for the scenario that you are mentioning about. Also Context has EntityProcessor / DataSource / VariableResolver as return types .What would be the right way to serialize the Java objects to the JavaSCript realm .
        Hide
        Noble Paul added a comment -

        All the Objects can be consumed directly from JS , just the way we use it in java .
        example

        <dataConfig>
                <script><![CDATA[
                        function f1(row, context)        {
                            var cmd =  context.getVariableResolver().replaceTokens('${dataimporter.request.command}');
                            if(cmd == 'full-import')
                                row.put('message', 'The command is full-import');
                            return row;
                        }
                ]]></script>
                <document>
                        <entity name="e" pk="id" transformer="script:f1" query="select * from X">
                        ....
                        </entity>
                </document>
        </dataConfig>
        

        Currently only a Transformer can be written in JS and that is the most common extension point.

        It is hard to write a DataSource/EntityProcessor in JS . I'm interested in having an Entityprocessor written in non-java. but there has not been much interest in it.

        an Eventlistener can be easily written in JS (but not yet implemented)

        Show
        Noble Paul added a comment - All the Objects can be consumed directly from JS , just the way we use it in java . example <dataConfig> <script><![CDATA[ function f1(row, context) { var cmd = context.getVariableResolver().replaceTokens('${dataimporter.request.command}'); if (cmd == 'full- import ') row.put('message', 'The command is full- import '); return row; } ]]></script> <document> <entity name= "e" pk= "id" transformer= "script:f1" query= "select * from X" > .... </entity> </document> </dataConfig> Currently only a Transformer can be written in JS and that is the most common extension point. It is hard to write a DataSource/EntityProcessor in JS . I'm interested in having an Entityprocessor written in non-java. but there has not been much interest in it. an Eventlistener can be easily written in JS (but not yet implemented)
        Hide
        Karthik K added a comment -
        It was not made an enum because I wamted them to be used in non-java languages (like javascript)

        Going back to the original issue - I am sure there would be some way to map the enum objects in java to javascript when it comes to porting the same. Until then ignoring type-safety seems to be ripe for errors ( A good number of test cases seem to pass a 0, as of today in place of ImportProcess in the ContextImpl ctor. when 0 makes no sense at all and another piece of code uses a -1 to check if it is an invalid process i.e. other than full import, delta import, find delta etc. ).

        Show
        Karthik K added a comment - It was not made an enum because I wamted them to be used in non-java languages (like javascript) Going back to the original issue - I am sure there would be some way to map the enum objects in java to javascript when it comes to porting the same. Until then ignoring type-safety seems to be ripe for errors ( A good number of test cases seem to pass a 0, as of today in place of ImportProcess in the ContextImpl ctor. when 0 makes no sense at all and another piece of code uses a -1 to check if it is an invalid process i.e. other than full import, delta import, find delta etc. ).
        Hide
        Noble Paul added a comment -

        I wished it had been String . But, it was already exposed in the API so I did not want to break it. (It may not be so widely used)

        Strings are portable across languages.

        Show
        Noble Paul added a comment - I wished it had been String . But, it was already exposed in the API so I did not want to break it. (It may not be so widely used) Strings are portable across languages.
        Hide
        Karthik K added a comment -

        I agree String-s are portable. But choosing an enum would still be orthogonal to Strings thanks to the Enum.valueOf(String) method - We can still regenerate the enum from the String representation (in case of non-Java languages).

        In any case - Context seems to be restricted to DIH and seems to be available since 1.3 only . So making the change now might be more intuitive to the developer instead of misusing with integers not meant to be passed as an argument in the first place.

        Show
        Karthik K added a comment - I agree String-s are portable. But choosing an enum would still be orthogonal to Strings thanks to the Enum.valueOf(String) method - We can still regenerate the enum from the String representation (in case of non-Java languages). In any case - Context seems to be restricted to DIH and seems to be available since 1.3 only . So making the change now might be more intuitive to the developer instead of misusing with integers not meant to be passed as an argument in the first place.
        Hide
        Shalin Shekhar Mangar added a comment -

        I see no advantage in maintaining an enum as well as a string. I agree that an integer is not intuitive and should be replaced by a string. It might be OK to break compatibility.

        Show
        Shalin Shekhar Mangar added a comment - I see no advantage in maintaining an enum as well as a string. I agree that an integer is not intuitive and should be replaced by a string. It might be OK to break compatibility.
        Hide
        Shalin Shekhar Mangar added a comment -

        Patch to change integer to strings.

        Show
        Shalin Shekhar Mangar added a comment - Patch to change integer to strings.
        Hide
        Shalin Shekhar Mangar added a comment -

        Patch in sync with trunk.

        I'll commit this shortly.

        Show
        Shalin Shekhar Mangar added a comment - Patch in sync with trunk. I'll commit this shortly.
        Hide
        Shalin Shekhar Mangar added a comment -

        Committed revision 765499.

        I changed the javadocs of Context.currentProcess to reflect the changes.

        Show
        Shalin Shekhar Mangar added a comment - Committed revision 765499. I changed the javadocs of Context.currentProcess to reflect the changes.
        Hide
        Grant Ingersoll added a comment -

        Bulk close Solr 1.4 issues

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

          People

          • Assignee:
            Shalin Shekhar Mangar
            Reporter:
            Karthik K
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

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

                Development