Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-9452

JsonRecordReader should not deep copy before handler.handle()

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.3, master (7.0)
    • Component/s: update
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      JsonRecordReader does a deep copy of the document map before calling handler.handle() method but it is not required because it is consumed in the same thread and not stored anywhere. The only place which needs a deep copy is the JsonRecordReader#getAllRecords method (used mostly for testing). Any such method can perform deep copy itself so that the common case is not penalized.

      This will save allocation of one copy of the map for each document.

      1. SOLR-9452.patch
        2 kB
        Shalin Shekhar Mangar
      2. SOLR-9452.patch
        2 kB
        Noble Paul
      3. SOLR-9452.patch
        2 kB
        Shalin Shekhar Mangar

        Activity

        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        Fix which moves the deep copy to the getAllRecords() method. Also, I removed the getDeepCopy method inside JsonRecordReader.Node and switched to using Utils.getDeepCopy method.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - Fix which moves the deep copy to the getAllRecords() method. Also, I removed the getDeepCopy method inside JsonRecordReader.Node and switched to using Utils.getDeepCopy method.
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        This patch fails the JsonLoaderTest

        java.lang.AssertionError: 
        Expected :null
        Actual   :c1
         <Click to see difference>
        
        
        	at __randomizedtesting.SeedInfo.seed([9D742CD9111942BA:383F0AD4B762BD16]:0)
        	at org.junit.Assert.fail(Assert.java:93)
        	at org.junit.Assert.failNotEquals(Assert.java:647)
        	at org.junit.Assert.assertEquals(Assert.java:128)
        	at org.junit.Assert.assertEquals(Assert.java:147)
        	at org.apache.solr.handler.JsonLoaderTest.testJsonDocFormat(JsonLoaderTest.java:381)
        	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        	at java.lang.reflect.Method.invoke(Method.java:498)
        

        Noble Paul - any ideas?

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - This patch fails the JsonLoaderTest java.lang.AssertionError: Expected : null Actual :c1 <Click to see difference> at __randomizedtesting.SeedInfo.seed([9D742CD9111942BA:383F0AD4B762BD16]:0) at org.junit.Assert.fail(Assert.java:93) at org.junit.Assert.failNotEquals(Assert.java:647) at org.junit.Assert.assertEquals(Assert.java:128) at org.junit.Assert.assertEquals(Assert.java:147) at org.apache.solr.handler.JsonLoaderTest.testJsonDocFormat(JsonLoaderTest.java:381) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) Noble Paul - any ideas?
        Hide
        noble.paul Noble Paul added a comment -

        I'll debug it

        Show
        noble.paul Noble Paul added a comment - I'll debug it
        Hide
        noble.paul Noble Paul added a comment -

        need deep copy for child docs as well. They are added to the childDocuments list

        Show
        noble.paul Noble Paul added a comment - need deep copy for child docs as well. They are added to the childDocuments list
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        I had an offline conversation with Noble Paul about why the deep copy of children is required and it is due to the fact that we keep the child map as list inside the main document's map. However, copying upto 2 levels is sufficient.

        All tests pass. I'll commit this shortly.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - I had an offline conversation with Noble Paul about why the deep copy of children is required and it is due to the fact that we keep the child map as list inside the main document's map. However, copying upto 2 levels is sufficient. All tests pass. I'll commit this shortly.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit f0f92d875ecab8a9acc01e959b852faf99a41e8e in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f0f92d8 ]

        SOLR-9452: JsonRecordReader should not deep copy document before handler.handle()

        Show
        jira-bot ASF subversion and git services added a comment - Commit f0f92d875ecab8a9acc01e959b852faf99a41e8e in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f0f92d8 ] SOLR-9452 : JsonRecordReader should not deep copy document before handler.handle()
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 27586ff82c7495cbe7d4ab9b531ed7e03eb48f20 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=27586ff ]

        SOLR-9452: JsonRecordReader should not deep copy document before handler.handle()
        (cherry picked from commit f0f92d8)

        Show
        jira-bot ASF subversion and git services added a comment - Commit 27586ff82c7495cbe7d4ab9b531ed7e03eb48f20 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=27586ff ] SOLR-9452 : JsonRecordReader should not deep copy document before handler.handle() (cherry picked from commit f0f92d8)
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        Thanks Noble!

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - Thanks Noble!
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 23825b248039c907d7eccc9b9fe381f836076539 in lucene-solr's branch refs/heads/master from Noble Paul
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=23825b2 ]

        SOLR-9452: Altered javadocs to reflect the new behavior

        Show
        jira-bot ASF subversion and git services added a comment - Commit 23825b248039c907d7eccc9b9fe381f836076539 in lucene-solr's branch refs/heads/master from Noble Paul [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=23825b2 ] SOLR-9452 : Altered javadocs to reflect the new behavior
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit da286a22c5f0467efaf0b296d5c120df5e698d26 in lucene-solr's branch refs/heads/branch_6x from Noble Paul
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=da286a2 ]

        SOLR-9452: Altered javadocs to reflect the new behavior

        Show
        jira-bot ASF subversion and git services added a comment - Commit da286a22c5f0467efaf0b296d5c120df5e698d26 in lucene-solr's branch refs/heads/branch_6x from Noble Paul [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=da286a2 ] SOLR-9452 : Altered javadocs to reflect the new behavior
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        Closing after 6.3.0 release.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - Closing after 6.3.0 release.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development