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

PHPSerialized fails with sharded queries

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3, 1.4.1
    • Fix Version/s: 3.1, 4.0-ALPHA
    • Component/s: Response Writers
    • Labels:
      None

      Description

      Solr throws a "java.lang.IllegalArgumentException: Map size must not be negative exception" when using the PHP Serialized response writer with sharded queries.
      To reproduce the issue start your preferred example and try the following query:

      http://localhost:8983/solr/select/?q=*:*&wt=phps&shards=localhost:8983/solr,localhost:8983/solr

      It is caused by the JSONWriter implementation of writeSolrDocumentList and writeSolrDocument. Overriding this two methods in the PHPSerializedResponseWriter to handle the SolrDocument size seems to solve the issue.
      Attached my patch made against trunk rev 1055588.

      cheers,
      Antonio

      1. PHPSerializedResponseWriter.java.patch
        3 kB
        Antonio Verni
      2. PHPSerializedResponseWriter.java.patch
        3 kB
        Antonio Verni
      3. TestPHPSerializedResponseWriter.java
        3 kB
        Antonio Verni
      4. PHPSerializedResponseWriter.java.patch
        3 kB
        Antonio Verni
      5. TestPHPSerializedResponseWriter.java
        3 kB
        Antonio Verni
      6. SOLR-2307.patch
        9 kB
        Hoss Man

        Issue Links

          Activity

          Hide
          anto Antonio Verni added a comment -

          Patch update to handle single values in multi valued fields. Added a junit test case just to test the issue.

          Show
          anto Antonio Verni added a comment - Patch update to handle single values in multi valued fields. Added a junit test case just to test the issue.
          Hide
          iorixxx Ahmet Arslan added a comment -

          This patch solves this problem http://search-lucene.com/m/lr7t42uWp4g , right?

          Show
          iorixxx Ahmet Arslan added a comment - This patch solves this problem http://search-lucene.com/m/lr7t42uWp4g , right?
          Hide
          anto Antonio Verni added a comment -

          yes, exactly, and it could also fix SOLR-2278 but I didn't tested it.
          As I discovered reading your SOLR-2291, the current patch I uploaded does not respect returnFields parameter. I've fixed it but I need to upload a third version of the patch (so please, sorry the mess) and a new test file.
          In detail, to fix the issue I've added a numeric index to the "response" array of documents in writeSolrDocumentList as requested by the php serialization protocol and handled the field count in writeSolrDocument.

          Show
          anto Antonio Verni added a comment - yes, exactly, and it could also fix SOLR-2278 but I didn't tested it. As I discovered reading your SOLR-2291 , the current patch I uploaded does not respect returnFields parameter. I've fixed it but I need to upload a third version of the patch (so please, sorry the mess) and a new test file. In detail, to fix the issue I've added a numeric index to the "response" array of documents in writeSolrDocumentList as requested by the php serialization protocol and handled the field count in writeSolrDocument.
          Hide
          iorixxx Ahmet Arslan added a comment -

          By the way, I think you should call req.close(); at the end the test.

          Show
          iorixxx Ahmet Arslan added a comment - By the way, I think you should call req.close(); at the end the test.
          Hide
          hossman Hoss Man added a comment -

          path looks good – includes a test that fails (horrificly) w/o patch applied.

          new attachment just merges the Antonio's two files (FYI: Antonio, if you do an "svn add" to your local copy, then "svn diff" will generate a unified patch even for new files); moves some existing PHPS test out of JSONWriterTest into the new test class, and adds the request closing that Ahmet mentioned.

          committing soon

          Show
          hossman Hoss Man added a comment - path looks good – includes a test that fails (horrificly) w/o patch applied. new attachment just merges the Antonio's two files (FYI: Antonio, if you do an "svn add" to your local copy, then "svn diff" will generate a unified patch even for new files); moves some existing PHPS test out of JSONWriterTest into the new test class, and adds the request closing that Ahmet mentioned. committing soon
          Hide
          hossman Hoss Man added a comment -

          Committed revision 1060585. – trunk

          Committed revision 1060589. - 3x.

          thanks again for the great patch Antonio

          Show
          hossman Hoss Man added a comment - Committed revision 1060585. – trunk Committed revision 1060589. - 3x. thanks again for the great patch Antonio
          Hide
          hossman Hoss Man added a comment -

          Uwe noticed a bug on some VMs ... the test assumes consistent iteration order of fields but that is not guaranteed by the code ... reopening to fix (should be straight forward)

          Show
          hossman Hoss Man added a comment - Uwe noticed a bug on some VMs ... the test assumes consistent iteration order of fields but that is not guaranteed by the code ... reopening to fix (should be straight forward)
          Hide
          hossman Hoss Man added a comment -

          code/test fixes to use predictable field order...

          Committed revision 1060641. - trunk
          Committed revision 1060642. - 3x

          Show
          hossman Hoss Man added a comment - code/test fixes to use predictable field order... Committed revision 1060641. - trunk Committed revision 1060642. - 3x
          Hide
          hossman Hoss Man added a comment -

          more fixes because the test itself also uses a HashMap that i didn't notice...

          Committed revision 1060645. trunk
          Committed revision 1060646. 3x

          Show
          hossman Hoss Man added a comment - more fixes because the test itself also uses a HashMap that i didn't notice... Committed revision 1060645. trunk Committed revision 1060646. 3x
          Hide
          hossman Hoss Man added a comment -

          i think we're all good here now

          Show
          hossman Hoss Man added a comment - i think we're all good here now
          Hide
          iorixxx Ahmet Arslan added a comment -

          Hoss, can you attach committed patch? I was wondering about predictable field order solution.

          Show
          iorixxx Ahmet Arslan added a comment - Hoss, can you attach committed patch? I was wondering about predictable field order solution.
          Hide
          gsingers Grant Ingersoll added a comment -

          Bulk close for 3.1.0 release

          Show
          gsingers Grant Ingersoll added a comment - Bulk close for 3.1.0 release

            People

            • Assignee:
              hossman Hoss Man
              Reporter:
              anto Antonio Verni
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development