Solr
  1. Solr
  2. SOLR-2307

PHPSerialized fails with sharded queries

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
          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
          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
          Ahmet Arslan added a comment -

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

          Show
          Ahmet Arslan added a comment - This patch solves this problem http://search-lucene.com/m/lr7t42uWp4g , right?
          Hide
          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
          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
          Ahmet Arslan added a comment -

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

          Show
          Ahmet Arslan added a comment - By the way, I think you should call req.close(); at the end the test.
          Hide
          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
          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
          Hoss Man added a comment -

          Committed revision 1060585. – trunk

          Committed revision 1060589. - 3x.

          thanks again for the great patch Antonio

          Show
          Hoss Man added a comment - Committed revision 1060585. – trunk Committed revision 1060589. - 3x. thanks again for the great patch Antonio
          Hide
          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
          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
          Hoss Man added a comment -

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

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

          Show
          Hoss Man added a comment - code/test fixes to use predictable field order... Committed revision 1060641. - trunk Committed revision 1060642. - 3x
          Hide
          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
          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
          Hoss Man added a comment -

          i think we're all good here now

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

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

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

          Bulk close for 3.1.0 release

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

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development