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

Switch Solr's Default Response Type from XML to JSON

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 7.0
    • Fix Version/s: 7.0, 7.1, master (8.0)
    • Component/s: Response Writers
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      Solr's default response format is still XML, despite the fact that Solr has supported the JSON response format for over a decade, developer mindshare has clearly shifted toward JSON over the years, and most modern/competing systems also use JSON format now by default.

      In fact, Solr's admin UI even explicitly adds wt=json to the request (by default in the UI) to override the default of wt=xml, so Solr's Admin UI effectively has a different default than the API.

      We have now introduced things like the JSON faceting API, and the new more modern /V2 apis assume JSON for the areas of Solr they cover, so clearly we're moving in the direction of JSON anyway.

      I'd like propose that we switch the default response writer to JSON (wt=json) instead of XML for Solr 7.0, as this seems to me like the right direction and a good time to make this change with the next major version.

      Based upon feedback from the Lucene Dev's mailing list, we want to:
      1) Change the default response writer type to "wt=json" and also change to "indent=on" by default
      2) Make no changes on the update handler side; it already works as desired (it returns the response in the same content-type as the request unless the "wt" is passed in explicitly).
      3) Keep the /query request handler around since people have already used it for years to do JSON queries
      4) Add a commented-out "wt=xml" to the solrconfig.xml as a reminder for folks on how to change (back) the response format.

      The default format change, plus the addition of "indent=on" are back compat changes, so we need to make sure we doc those clearly in the CHANGES.txt. There will also need to be significant adjustments to the Solr Ref Guide, Tutorial, etc.

      1. SOLR-10494
        11 kB
        Trey Grainger
      2. SOLR-10494
        10 kB
        Trey Grainger
      3. SOLR-10494.branch_7x.patch
        164 kB
        Trey Grainger
      4. SOLR-10494.patch
        162 kB
        Hoss Man
      5. SOLR-10494.patch
        162 kB
        Hoss Man
      6. SOLR-10494.patch
        164 kB
        Trey Grainger
      7. SOLR-10494-withdocs.patch
        44 kB
        Cassandra Targett
      8. SOLR-10494-withdocs.patch
        44 kB
        Cassandra Targett

        Issue Links

          Activity

          Hide
          shalinmangar Shalin Shekhar Mangar added a comment -

          Bulk close after 7.1.0 release

          Show
          shalinmangar Shalin Shekhar Mangar added a comment - Bulk close after 7.1.0 release
          Hide
          hossman Hoss Man added a comment -

          this change fixed smoketest for me on master, so i went ahead and backported it – but i'm actually still sanity testing on 7x/7_0

          Show
          hossman Hoss Man added a comment - this change fixed smoketest for me on master, so i went ahead and backported it – but i'm actually still sanity testing on 7x/7_0
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 34e3ef7d7b0823085ec34166af13ced1df56e09a in lucene-solr's branch refs/heads/branch_7x from Chris Hostetter
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=34e3ef7 ]

          SOLR-10494: fix smoker

          (cherry picked from commit 56ad1a7a9b049e7399c0e482dcf6f4f395472f5b)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 34e3ef7d7b0823085ec34166af13ced1df56e09a in lucene-solr's branch refs/heads/branch_7x from Chris Hostetter [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=34e3ef7 ] SOLR-10494 : fix smoker (cherry picked from commit 56ad1a7a9b049e7399c0e482dcf6f4f395472f5b)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 25637fbba37e5c339562748e4bddcde31869ca2b in lucene-solr's branch refs/heads/branch_7_0 from Chris Hostetter
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=25637fb ]

          SOLR-10494: fix smoker

          (cherry picked from commit 56ad1a7a9b049e7399c0e482dcf6f4f395472f5b)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 25637fbba37e5c339562748e4bddcde31869ca2b in lucene-solr's branch refs/heads/branch_7_0 from Chris Hostetter [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=25637fb ] SOLR-10494 : fix smoker (cherry picked from commit 56ad1a7a9b049e7399c0e482dcf6f4f395472f5b)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 56ad1a7a9b049e7399c0e482dcf6f4f395472f5b in lucene-solr's branch refs/heads/master from Chris Hostetter
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=56ad1a7 ]

          SOLR-10494: fix smoker

          Show
          jira-bot ASF subversion and git services added a comment - Commit 56ad1a7a9b049e7399c0e482dcf6f4f395472f5b in lucene-solr's branch refs/heads/master from Chris Hostetter [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=56ad1a7 ] SOLR-10494 : fix smoker
          Hide
          hossman Hoss Man added a comment -

          gah ... sorry ... i didn't even think about that.

          lemme review the rest of the smoketester a bit more and figure out if it makes more sense to just add wt=xml, or to redo these bits to expect json by default.

          Show
          hossman Hoss Man added a comment - gah ... sorry ... i didn't even think about that. lemme review the rest of the smoketester a bit more and figure out if it makes more sense to just add wt=xml, or to redo these bits to expect json by default.
          Hide
          steve_rowe Steve Rowe added a comment -

          Smoke tester is unhappy on master because a Solr query response is in JSON vs XML - from https://builds.apache.org/job/Lucene-Solr-SmokeRelease-master/823/:

          [...]
            [smoker] Solr techproducts example launched successfully. Direct your Web browser to http://localhost:8983/solr to visit the Solr Admin UI
             [smoker]       test utf8...
             [smoker]       run query...
             [smoker] FAILED: response is:
             [smoker] {
             [smoker]   "responseHeader":{
             [smoker]     "status":0,
             [smoker]     "QTime":0,
             [smoker]     "params":{
             [smoker]       "q":"video"}},
             [smoker]   "response":{"numFound":3,"start":0,"docs":[
          [...]
          

          From smokeTestRelease.py:

          859: print('      run query...')
          860: s = load('http://localhost:8983/solr/techproducts/select/?q=video')
          861: if s.find('<result name="response" numFound="3" start="0">') == -1:
          862:   print('FAILED: response is:\n%s' % s)
          
          Show
          steve_rowe Steve Rowe added a comment - Smoke tester is unhappy on master because a Solr query response is in JSON vs XML - from https://builds.apache.org/job/Lucene-Solr-SmokeRelease-master/823/ : [...] [smoker] Solr techproducts example launched successfully. Direct your Web browser to http://localhost:8983/solr to visit the Solr Admin UI [smoker] test utf8... [smoker] run query... [smoker] FAILED: response is: [smoker] { [smoker] "responseHeader":{ [smoker] "status":0, [smoker] "QTime":0, [smoker] "params":{ [smoker] "q":"video"}}, [smoker] "response":{"numFound":3,"start":0,"docs":[ [...] From smokeTestRelease.py : 859: print(' run query...') 860: s = load('http: //localhost:8983/solr/techproducts/select/?q=video') 861: if s.find('<result name= "response" numFound= "3" start= "0" >') == -1: 862: print('FAILED: response is:\n%s' % s)
          Hide
          hossman Hoss Man added a comment -

          backport was while jira was down, so gitbot didn't note all the commits...

          master - 6a59253ec34e9e08d6b2306b51c81199d3f3d828
          7x - 8ea4c0790d003efafe893db3d6ab33ad494a1213
          7_0 - 477c2188ef9f2c82902cf4ed1ccb91bfc6ab5ebf

          Show
          hossman Hoss Man added a comment - backport was while jira was down, so gitbot didn't note all the commits... master - 6a59253ec34e9e08d6b2306b51c81199d3f3d828 7x - 8ea4c0790d003efafe893db3d6ab33ad494a1213 7_0 - 477c2188ef9f2c82902cf4ed1ccb91bfc6ab5ebf
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 6a59253ec34e9e08d6b2306b51c81199d3f3d828 in lucene-solr's branch refs/heads/master from Chris Hostetter
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6a59253 ]

          SOLR-10494: Make default response format JSON (wt=json), and also indent text responses formats (indent=on) by default

          Show
          jira-bot ASF subversion and git services added a comment - Commit 6a59253ec34e9e08d6b2306b51c81199d3f3d828 in lucene-solr's branch refs/heads/master from Chris Hostetter [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6a59253 ] SOLR-10494 : Make default response format JSON (wt=json), and also indent text responses formats (indent=on) by default
          Hide
          hossman Hoss Man added a comment -

          Jan Høydahl: FWIW, I still haven't reviewed the patch in depth, and I'm leaving for a 2 week vacation in a few days (w/limited bandwidth between now and then).

          If you already reviewed the meat of the patch and were satisfied with the changes but just got blocked on the test failures before you were ready to commit, then by all means please proceed with committing – but you may want to re-review the ref-guide changes since i had to resolve a lot of patch merge conflicts there.

          Show
          hossman Hoss Man added a comment - Jan Høydahl : FWIW, I still haven't reviewed the patch in depth, and I'm leaving for a 2 week vacation in a few days (w/limited bandwidth between now and then). If you already reviewed the meat of the patch and were satisfied with the changes but just got blocked on the test failures before you were ready to commit, then by all means please proceed with committing – but you may want to re-review the ref-guide changes since i had to resolve a lot of patch merge conflicts there.
          Hide
          hossman Hoss Man added a comment -

          ...I'm pretty sure the root problem here is that with the change to default to indent=true, really deep XML docs are including more newlines in places they weren't before, and that's causing some bugs in the SOlrJ XMLResponseParse ...

          The scope of the bug was much more narrow then i realized: it only affected nested child docs when indent=true (newlines had nothing to do with it) – fixed now in SOLR-11136.


          I tried to debug theTestHierarchicalDocBuilder.testThreeLevelHierarchy test failure, and I see that the test data is different but that may be the intention, to randomize the hierarchy.

          that failure is actually really simple: the xpath is badly written. it's using syntax like /arr[@name='id' and .='"parentId1"'] which only works if all of the accumlated text data inside the <arr> equal parentId after they've been concatenated – but when indent=true there are additional (whitespace) text data nodes in the DOM before and after the <str>...</str>.

          In the attached path i simplified & tightened up these xpaths to be explicit about looking for the <str>...</str> content.

          (NOTE: still haven't actaully reviewed the patch, just focused on getting the tests to pass)

          Show
          hossman Hoss Man added a comment - ...I'm pretty sure the root problem here is that with the change to default to indent=true, really deep XML docs are including more newlines in places they weren't before, and that's causing some bugs in the SOlrJ XMLResponseParse ... The scope of the bug was much more narrow then i realized: it only affected nested child docs when indent=true (newlines had nothing to do with it) – fixed now in SOLR-11136 . I tried to debug theTestHierarchicalDocBuilder.testThreeLevelHierarchy test failure, and I see that the test data is different but that may be the intention, to randomize the hierarchy. that failure is actually really simple: the xpath is badly written. it's using syntax like /arr[@name='id' and .='" parentId1 "'] which only works if all of the accumlated text data inside the <arr> equal parentId after they've been concatenated – but when indent=true there are additional (whitespace) text data nodes in the DOM before and after the <str>...</str> . In the attached path i simplified & tightened up these xpaths to be explicit about looking for the <str>...</str> content. (NOTE: still haven't actaully reviewed the patch, just focused on getting the tests to pass)
          Hide
          hossman Hoss Man added a comment -

          Here's an update patch against master (58358099bf72b) ... most of the conflicts were in the ref guide, and i'm pretty sure i captured the intent of all the cahnges – but i haven't reviewed that part in depth.

          In truth, i haven't reviewed much of the patch at all – i've been looking into the test failures. I'm pretty sure the root problem here is that with the change to default to indent=true, really deep XML docs are including more newlines in places they weren't before, and that's causing some bugs in the SOlrJ XMLResponseParser because i think it causes the XMLStreamReader to consider that "non-ignorable" whitespace so it's returning it as CHARACTER data in places where the parser doesn't expect CHARACTER data.

          this seems like a legic bug that we should fix as a blocker to making these changes – i'll dig in more and file a new jira once i have a more concrete diagnoses

          Show
          hossman Hoss Man added a comment - Here's an update patch against master (58358099bf72b) ... most of the conflicts were in the ref guide, and i'm pretty sure i captured the intent of all the cahnges – but i haven't reviewed that part in depth. In truth, i haven't reviewed much of the patch at all – i've been looking into the test failures. I'm pretty sure the root problem here is that with the change to default to indent=true, really deep XML docs are including more newlines in places they weren't before, and that's causing some bugs in the SOlrJ XMLResponseParser because i think it causes the XMLStreamReader to consider that "non-ignorable" whitespace so it's returning it as CHARACTER data in places where the parser doesn't expect CHARACTER data. this seems like a legic bug that we should fix as a blocker to making these changes – i'll dig in more and file a new jira once i have a more concrete diagnoses
          Hide
          solrtrey Trey Grainger added a comment -

          Here's the most up-to-date patch against branch_7x.

          Show
          solrtrey Trey Grainger added a comment - Here's the most up-to-date patch against branch_7x.
          Hide
          solrtrey Trey Grainger added a comment - - edited

          Hi Jan Høydahl.

          I picked it up a few times, but was developing against master and kept running into stability issues with other tests every time I pulled. I finally switched over to just developing on the 7.x branch instead to prevent those stability issues. I have an updated patch which fixes some (now) merge conflicts with the default configset changes, and all tests appear to be passing except the TestHierarchicalDocBuilder.testThreeLevelHierarchy one. I still haven't been able to dig deep enough to understand what is effecting that one.

          I DO know that the issues is related to indention. If I go into the test and override it to "indent=off" then it succeeds, but I have no idea why indention being on is causing the failure. Also, doing that in the test is probably just masking another underlying problem, which may not even be test related, so I really need to understand exactly where things are breaking down to know if it's a test problem or an actual functionality problem somewhere.

          At any rate, I'll post my updated patch here shortly. I'm a little tight on time this next week, so hopefully I can enlist someone else to assist on my end later today, as well.

          Show
          solrtrey Trey Grainger added a comment - - edited Hi Jan Høydahl . I picked it up a few times, but was developing against master and kept running into stability issues with other tests every time I pulled. I finally switched over to just developing on the 7.x branch instead to prevent those stability issues. I have an updated patch which fixes some (now) merge conflicts with the default configset changes, and all tests appear to be passing except the TestHierarchicalDocBuilder.testThreeLevelHierarchy one. I still haven't been able to dig deep enough to understand what is effecting that one. I DO know that the issues is related to indention. If I go into the test and override it to "indent=off" then it succeeds, but I have no idea why indention being on is causing the failure. Also, doing that in the test is probably just masking another underlying problem, which may not even be test related, so I really need to understand exactly where things are breaking down to know if it's a test problem or an actual functionality problem somewhere. At any rate, I'll post my updated patch here shortly. I'm a little tight on time this next week, so hopefully I can enlist someone else to assist on my end later today, as well.
          Hide
          janhoy Jan Høydahl added a comment -

          So, any progress on this so we have a chance of getting it into 7.0.0? I don't have the chance to work on it the next 4 weeks

          Show
          janhoy Jan Høydahl added a comment - So, any progress on this so we have a chance of getting it into 7.0.0? I don't have the chance to work on it the next 4 weeks
          Hide
          janhoy Jan Høydahl added a comment -

          I tried to debug theTestHierarchicalDocBuilder.testThreeLevelHierarchy test failure, and I see that the test data is different but that may be the intention, to randomize the hierarchy. The patch is pretty large so I guess several things could affect this. At least the failures are 100% reproducible so it should be possible to get to the end of it.

          Show
          janhoy Jan Høydahl added a comment - I tried to debug theTestHierarchicalDocBuilder.testThreeLevelHierarchy test failure, and I see that the test data is different but that may be the intention, to randomize the hierarchy. The patch is pretty large so I guess several things could affect this. At least the failures are 100% reproducible so it should be possible to get to the end of it.
          Hide
          solrtrey Trey Grainger added a comment - - edited

          Ok, I think I'm nearly done. This patch (SOLR-10494.patch) includes removing all the extraneous "wt=json" and "indent=on" references, adding a commented out version of "wt=xml" to the example solrconfig.xml's, unit test updates, some additional updates to the tutorials and docs (also incorporating Cassandra Targett's), and updating the admin UI (query section) to handle the new defaults.

          The only issue I'm running into is that for some reason I haven't figured out yet, turning "indent" on has broken some of the parent/child relationship tests (i.e. TestHierarchicalDocBuilder.testThreeLevelHierarchy, SolrExampleTests.testChildDocTransformer. It initially appears to be some xml parsing issue issue with the extra whitespace, which would be odd, but I haven't dug in yet. Once I figure those out, I'll update the patch, and then I think this will be ready for review.

          Show
          solrtrey Trey Grainger added a comment - - edited Ok, I think I'm nearly done. This patch ( SOLR-10494.patch ) includes removing all the extraneous "wt=json" and "indent=on" references, adding a commented out version of "wt=xml" to the example solrconfig.xml's, unit test updates, some additional updates to the tutorials and docs (also incorporating Cassandra Targett 's), and updating the admin UI (query section) to handle the new defaults. The only issue I'm running into is that for some reason I haven't figured out yet, turning "indent" on has broken some of the parent/child relationship tests (i.e. TestHierarchicalDocBuilder.testThreeLevelHierarchy, SolrExampleTests.testChildDocTransformer. It initially appears to be some xml parsing issue issue with the extra whitespace, which would be odd, but I haven't dug in yet. Once I figure those out, I'll update the patch, and then I think this will be ready for review.
          Hide
          solrtrey Trey Grainger added a comment -

          Also should we mark this as a blocker for 7.0 to change it? - Varun Thacker

          I just updated it to be a blocker, Varun. I'm working on what should be the final patch today. Hopefully this can be reviewed and make it in for 7.0.

          Show
          solrtrey Trey Grainger added a comment - Also should we mark this as a blocker for 7.0 to change it? - Varun Thacker I just updated it to be a blocker, Varun. I'm working on what should be the final patch today. Hopefully this can be reviewed and make it in for 7.0.
          Hide
          solrtrey Trey Grainger added a comment -

          Thanks, Cassandra Targett! I'm building off you patch and making final changes. Been a bit slammed this week and am unavailable to work on this for the next 24-36 hours, but I expect to have the next (hopefully final, or close to it) patch pushed sometime on Sunday (in the U.S.).

          Show
          solrtrey Trey Grainger added a comment - Thanks, Cassandra Targett ! I'm building off you patch and making final changes. Been a bit slammed this week and am unavailable to work on this for the next 24-36 hours, but I expect to have the next (hopefully final, or close to it) patch pushed sometime on Sunday (in the U.S.).
          Hide
          janhoy Jan Høydahl added a comment -

          No worries This time it came out right

          Show
          janhoy Jan Høydahl added a comment - No worries This time it came out right
          Hide
          ctargett Cassandra Targett added a comment -

          Is it? Damn it. We need How To Contribute docs that don't assume every task is 2nd nature to everyone at all times.

          Second try attached. If it's still wrong I don't know what to do.

          Show
          ctargett Cassandra Targett added a comment - Is it? Damn it. We need How To Contribute docs that don't assume every task is 2nd nature to everyone at all times. Second try attached. If it's still wrong I don't know what to do.
          Hide
          janhoy Jan Høydahl added a comment -

          Good work Cassandra. Think your patch is reversed though

          Show
          janhoy Jan Høydahl added a comment - Good work Cassandra. Think your patch is reversed though
          Hide
          ctargett Cassandra Targett added a comment -

          Here's a patch with Trey Grainger's changes + ref guide changes. Nearly all of the changes needed were to simply remove &wt=json&indent=true from examples. Of course I found a couple unrelated glaring issues I couldn't let pass by, but I kept those to an extreme minimum.

          I reviewed all the places in the Ref Guide where wt is mentioned. Turns out we never actually said XML is the default...just said you had to use wt=json. I also updated the solrconfig.xml request handler defaults examples.

          One thing I neglected to look for were places where an example request does not specify wt=xml but the example response is XML. I can do another pass for those at a later time - I would guess there aren't a lot of those.

          Show
          ctargett Cassandra Targett added a comment - Here's a patch with Trey Grainger 's changes + ref guide changes. Nearly all of the changes needed were to simply remove &wt=json&indent=true from examples. Of course I found a couple unrelated glaring issues I couldn't let pass by, but I kept those to an extreme minimum. I reviewed all the places in the Ref Guide where wt is mentioned. Turns out we never actually said XML is the default...just said you had to use wt=json . I also updated the solrconfig.xml request handler defaults examples. One thing I neglected to look for were places where an example request does not specify wt=xml but the example response is XML. I can do another pass for those at a later time - I would guess there aren't a lot of those.
          Hide
          janhoy Jan Høydahl added a comment -

          That was just a IDEA search, see https://www.dropbox.com/s/4dan2pdt21d2t99/wt_json.png?dl=0
          Could be that all the uses in TestSolrConfigHandler.java are still necessary, or should that API also default to JSON response...

          Show
          janhoy Jan Høydahl added a comment - That was just a IDEA search, see https://www.dropbox.com/s/4dan2pdt21d2t99/wt_json.png?dl=0 Could be that all the uses in TestSolrConfigHandler.java are still necessary, or should that API also default to JSON response...
          Hide
          ctargett Cassandra Targett added a comment -

          I'm happy to help with doc/website updates. Some of the ref guide references might be big and would delay this change significantly (IOW, may make it miss 7.0).

          Re: quickstart - I proposed in SOLR-10842 to move that to the Ref Guide, and I'm working up a patch (maybe this week? it's a huge reorg). I think we could let that go from this patch in deference to this other effort, again to not hold up what should be a pretty minor change.

          Remove superflous wt=json from various places (There are hundreds, including Admin UI, READMEs etc)

          I could not find 100s of references. Jan Høydahl, Can you give a list, or the grep/sed/whatever that you used to find those?

          Show
          ctargett Cassandra Targett added a comment - I'm happy to help with doc/website updates. Some of the ref guide references might be big and would delay this change significantly (IOW, may make it miss 7.0). Re: quickstart - I proposed in SOLR-10842 to move that to the Ref Guide, and I'm working up a patch (maybe this week? it's a huge reorg). I think we could let that go from this patch in deference to this other effort, again to not hold up what should be a pretty minor change. Remove superflous wt=json from various places (There are hundreds, including Admin UI, READMEs etc) I could not find 100s of references. Jan Høydahl , Can you give a list, or the grep/sed/whatever that you used to find those?
          Hide
          solrtrey Trey Grainger added a comment -

          yes, I'll address all of the code/config changes above. I'll get the patch updated to include the indent=on change first (fixing unit tests now... were more that broke than I was expecting due to indention) and then do the cleanup of the configs, admin, readme's, as a follow on patch.

          Once those are in, I can take a look at the ref-guide, website, and quickstart, though I'm afraid I may need some help pull all of those off in any reasonable timeframe for 7.0, as I'd expect there to be a lot of changes required there.

          Show
          solrtrey Trey Grainger added a comment - yes, I'll address all of the code/config changes above. I'll get the patch updated to include the indent=on change first (fixing unit tests now... were more that broke than I was expecting due to indention) and then do the cleanup of the configs, admin, readme's, as a follow on patch. Once those are in, I can take a look at the ref-guide, website, and quickstart, though I'm afraid I may need some help pull all of those off in any reasonable timeframe for 7.0, as I'd expect there to be a lot of changes required there.
          Hide
          janhoy Jan Høydahl added a comment -

          Some comments

          • Could you add a .patch extension to the patch attachments for easier consumption?
          • Remove superflous <str name="wt">json</str> from various solrconfig.xml's (I counted 14 locations including ref-guide)
          • Remove superflous wt=json from various places (There are hundreds, including Admin UI, READMEs etc)
          • Document the new default in ref-guide
          • CHANGES entry
          • Changes to Solr website, most importantly Quickstart Tutorial
          Show
          janhoy Jan Høydahl added a comment - Some comments Could you add a .patch extension to the patch attachments for easier consumption? Remove superflous <str name="wt">json</str> from various solrconfig.xml's (I counted 14 locations including ref-guide) Remove superflous wt=json from various places (There are hundreds, including Admin UI, READMEs etc) Document the new default in ref-guide CHANGES entry Changes to Solr website, most importantly Quickstart Tutorial
          Hide
          noble.paul Noble Paul added a comment -

          We should also set json.nl=map as the default

          Show
          noble.paul Noble Paul added a comment - We should also set json.nl=map as the default
          Hide
          solrtrey Trey Grainger added a comment -

          New patch fixing a precommit error. Comment earlier about unclosed resources was apparently pre-existing (those are warnings and not errors) and I just noticed it because of an unrelated error, so going to ignore those. Working on indent=on by default for next patch.

          Show
          solrtrey Trey Grainger added a comment - New patch fixing a precommit error. Comment earlier about unclosed resources was apparently pre-existing (those are warnings and not errors) and I just noticed it because of an unrelated error, so going to ignore those. Working on indent=on by default for next patch.
          Hide
          solrtrey Trey Grainger added a comment -

          Initial patch, with all unit tests broken by the change now passing. Haven't changed to indent=on by default yet or removed setting of json explicitly in various places yet, though, as I've been trying to change one variable at a time to minimize complications.

          For some reason, switching to json by default has caused ant precommit to complain about resource leak in about 60 places. I'm not sure what is causing these at the moment, but want to address that first before adding any additional changes to the patch.

          Show
          solrtrey Trey Grainger added a comment - Initial patch, with all unit tests broken by the change now passing. Haven't changed to indent=on by default yet or removed setting of json explicitly in various places yet, though, as I've been trying to change one variable at a time to minimize complications. For some reason, switching to json by default has caused ant precommit to complain about resource leak in about 60 places. I'm not sure what is causing these at the moment, but want to address that first before adding any additional changes to the patch.
          Hide
          solrtrey Trey Grainger added a comment -

          Question: I'm making indent=on the default. Any objections if I make indent=on the default for all TextResponseWriters, or do I need to limit the change to only the "wt=json" (now default writer) case.

          The writers impacted from what I can tell are:
          GEOJSONWriter
          JSONWriter
          XMLWriter
          SchemaXMLWriter
          PHPWriter
          PythonWriter
          RubyWriter

          It's a little complicated because most of these (geojson, php, python, ruby) actually inherit from the JSONWriter, so if I need to leave indent=off on those then I have to go in and set it explicitly on them since their base class will now have indent on by default.

          Unless anyone objects, I'm just going to set indent=on by default on all of these. Please let me know if anyone disagrees.

          Show
          solrtrey Trey Grainger added a comment - Question: I'm making indent=on the default. Any objections if I make indent=on the default for all TextResponseWriters, or do I need to limit the change to only the "wt=json" (now default writer) case. The writers impacted from what I can tell are: GEOJSONWriter JSONWriter XMLWriter SchemaXMLWriter PHPWriter PythonWriter RubyWriter It's a little complicated because most of these (geojson, php, python, ruby) actually inherit from the JSONWriter, so if I need to leave indent=off on those then I have to go in and set it explicitly on them since their base class will now have indent on by default. Unless anyone objects, I'm just going to set indent=on by default on all of these. Please let me know if anyone disagrees.
          Hide
          solrtrey Trey Grainger added a comment -

          Started working on this two weeks ago and then got busy. The actual changes were super quick, but after I made them it was taking over 2 hours to run the unit tests with lots of failures and several test suites timing out.

          Just got back to this today and have pretty much everything diagnosed and am working on fixes. In short, SolrTestCaseJ4 has XPath checking hard-coded in its design, so I need to now pass in wt=xml explicitly there, and there are a handful of test suites (i.e. replication/backup/restore and hdfs) that are explicitly checking XML strings and looping forever until they get those strings back (hence timing out).

          I'm making changes to explicitly request XML right now for those tests where they are expecting it and will get a patch posted hopefully today.

          Show
          solrtrey Trey Grainger added a comment - Started working on this two weeks ago and then got busy. The actual changes were super quick, but after I made them it was taking over 2 hours to run the unit tests with lots of failures and several test suites timing out. Just got back to this today and have pretty much everything diagnosed and am working on fixes. In short, SolrTestCaseJ4 has XPath checking hard-coded in its design, so I need to now pass in wt=xml explicitly there, and there are a handful of test suites (i.e. replication/backup/restore and hdfs) that are explicitly checking XML strings and looping forever until they get those strings back (hence timing out). I'm making changes to explicitly request XML right now for those tests where they are expecting it and will get a patch posted hopefully today.
          Hide
          noble.paul Noble Paul added a comment -

          Yes, we should keep indent=on by default. There purpose of json output is to make the output more human readable

          Show
          noble.paul Noble Paul added a comment - Yes, we should keep indent=on by default. There purpose of json output is to make the output more human readable
          Hide
          varunthacker Varun Thacker added a comment -

          One observation noted in SOLR-10812 was although the V2 APIs standardize JSOn it didn't have indent=on . I guess that's also part of the scope for this Jira?

          Also should we mark this as a blocker for 7.0 to change it?

          Show
          varunthacker Varun Thacker added a comment - One observation noted in SOLR-10812 was although the V2 APIs standardize JSOn it didn't have indent=on . I guess that's also part of the scope for this Jira? Also should we mark this as a blocker for 7.0 to change it?
          Hide
          noble.paul Noble Paul added a comment - - edited

          +1 for json as the default response format

          some places in Solr use json as the default such as: config api, security api , streaming api, v2 API . These components have done explicit coding to make json the defualt. We need to remove those as a part of the cleanup

          Show
          noble.paul Noble Paul added a comment - - edited +1 for json as the default response format some places in Solr use json as the default such as: config api, security api , streaming api, v2 API . These components have done explicit coding to make json the defualt. We need to remove those as a part of the cleanup
          Hide
          solrtrey Trey Grainger added a comment -

          Hi Jan Høydahl. Sorry - I missed you first message last week. Sure - I should be able to get a patch posted this weekend.

          -Trey

          Show
          solrtrey Trey Grainger added a comment - Hi Jan Høydahl . Sorry - I missed you first message last week. Sure - I should be able to get a patch posted this weekend. -Trey
          Hide
          janhoy Jan Høydahl added a comment -

          Trey Grainger do you plan to put up a patch for this? Shouldn't bee a very big one..

          Show
          janhoy Jan Høydahl added a comment - Trey Grainger do you plan to put up a patch for this? Shouldn't bee a very big one..
          Hide
          janhoy Jan Høydahl added a comment -

          Solr 7 is just a month away, do you have time to work on this, Trey?

          Show
          janhoy Jan Høydahl added a comment - Solr 7 is just a month away, do you have time to work on this, Trey?
          Hide
          janhoy Jan Høydahl added a comment -

          +1

          Show
          janhoy Jan Høydahl added a comment - +1

            People

            • Assignee:
              hossman Hoss Man
              Reporter:
              solrtrey Trey Grainger
            • Votes:
              2 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development