Solr
  1. Solr
  2. SOLR-4234

Add support for binary files in ZooKeeper.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.0
    • Fix Version/s: 4.4, 6.0
    • Component/s: SolrCloud
    • Labels:
      None

      Description

      I was attempting to get the ShowFileHandler to show a .png file, and it was failing. But in non-ZK mode it worked just fine! It took a while, but it seems that we upload to zk as a text, and download as well. I've attached a unit test that demonstrates the problem, and a fix. You have to have a binary file in the conf directory to make the test work, I put solr.png in the collection1/conf/velocity directory.

      1. binary_upload_download.patch
        4 kB
        Eric Pugh
      2. fix_show_file_handler_with_binaries.patch
        2 kB
        Eric Pugh
      3. solr.png
        8 kB
        Eric Pugh
      4. SOLR4234_binary_files.patch
        7 kB
        Eric Pugh

        Activity

        Hide
        Eric Pugh added a comment -

        Testcase and patch for binary files in ZK.

        Show
        Eric Pugh added a comment - Testcase and patch for binary files in ZK.
        Hide
        Eric Pugh added a comment -

        This is the patch to allow the ShowFileHandler to take binaries.

        Show
        Eric Pugh added a comment - This is the patch to allow the ShowFileHandler to take binaries.
        Show
        Eric Pugh added a comment - Without this patch, a request like this would fail: http://localhost:8983/solr/my_search_core_shard1_replica1/admin/file?file=/velocity/header.png&contentType=image/png
        Hide
        Erik Hatcher added a comment -

        lol - while I like /admin/file and of course mixing and matching it with the VelocityResponseWriter, I cringe at ZK serving up favicons.

        But hey, better that it works for binary files than didn't, so +1

        One minor issue with the patch is the incorrect javadoc added to the ByteArrayStream ctor "Construct a <code>ContentStream</code> from a <code>File</code>", so let's get that correct.

        Show
        Erik Hatcher added a comment - lol - while I like /admin/file and of course mixing and matching it with the VelocityResponseWriter, I cringe at ZK serving up favicons. But hey, better that it works for binary files than didn't, so +1 One minor issue with the patch is the incorrect javadoc added to the ByteArrayStream ctor "Construct a <code>ContentStream</code> from a <code>File</code>", so let's get that correct.
        Hide
        Eric Pugh added a comment -

        Mark Miller, Erik Hatcher, are there any other changes needed to be made? And one nice thing about fixing the upload aspect of ZKCli, is that we can potentially load jar files from a Solr Core (./core/lib/myjar.jar), which is something that only non ZK mode works with. Right now, the ZKCli munges up the binary jar file when you upload to ZK.

        Show
        Eric Pugh added a comment - Mark Miller , Erik Hatcher , are there any other changes needed to be made? And one nice thing about fixing the upload aspect of ZKCli, is that we can potentially load jar files from a Solr Core (./core/lib/myjar.jar), which is something that only non ZK mode works with. Right now, the ZKCli munges up the binary jar file when you upload to ZK.
        Hide
        Mark Miller added a comment -

        Hey Eric - do you think there will be an issue with back compat here?

        Show
        Mark Miller added a comment - Hey Eric - do you think there will be an issue with back compat here?
        Hide
        Eric Pugh added a comment -

        Mark Miller you mean if I have already loaded data using the String method into ZK, swapping to bytes will cause issues?

        I'll go ahead and test that today by hand, and let you know. Do you think I need to craft a unit test that loads solrconfig as strings, and then uses the new byte approach?

        Show
        Eric Pugh added a comment - Mark Miller you mean if I have already loaded data using the String method into ZK, swapping to bytes will cause issues? I'll go ahead and test that today by hand, and let you know. Do you think I need to craft a unit test that loads solrconfig as strings, and then uses the new byte approach?
        Hide
        Mark Miller added a comment -

        Yeah, my main concern is that 4.0 cloud users will have trouble reading info from zk with 4.1. I'm not sure we necessarily need a unit test, but at least a verification that people can easily upgrade if we put this in.

        Show
        Mark Miller added a comment - Yeah, my main concern is that 4.0 cloud users will have trouble reading info from zk with 4.1. I'm not sure we necessarily need a unit test, but at least a verification that people can easily upgrade if we put this in.
        Hide
        Eric Pugh added a comment -

        Consolidated patch file, with ZkCLI, ShowFileRequestHandler, and demoing using Velocity /browse interface.

        Show
        Eric Pugh added a comment - Consolidated patch file, with ZkCLI, ShowFileRequestHandler, and demoing using Velocity /browse interface.
        Hide
        Eric Pugh added a comment -

        This solr.png is a copy of the main one, and needs to go in the /collection1/conf/velocity directory.

        Show
        Eric Pugh added a comment - This solr.png is a copy of the main one, and needs to go in the /collection1/conf/velocity directory.
        Hide
        Eric Pugh added a comment -

        Mark Miller Okay, I've gone through on Trunk and 4.x and tested out first loading the configuration files into ZooKeeper as UTF-8 String, and then doing it as bytes[] and everything DOES WORK!
        I have consolidated the patch into a single SOLR4234_binary_files.patch, but you do need to add solr.png to the /velocity directory.

        Show
        Eric Pugh added a comment - Mark Miller Okay, I've gone through on Trunk and 4.x and tested out first loading the configuration files into ZooKeeper as UTF-8 String, and then doing it as bytes[] and everything DOES WORK! I have consolidated the patch into a single SOLR4234_binary_files.patch, but you do need to add solr.png to the /velocity directory.
        Hide
        Mark Miller added a comment -

        Hey Eric, I"m seeing the following tests fail:

        java.lang.AssertionError: Make sure we did download each file in the original configuration
        at __randomizedtesting.SeedInfo.seed([BB2B109CDBABDCE7:9F4DBD9251646A81]:0)
        at org.junit.Assert.fail(Assert.java:93)
        at org.junit.Assert.assertTrue(Assert.java:43)
        at org.apache.solr.cloud.ZkCLITest.testUpConfigLinkConfigClearZk(ZkCLITest.java:182)

        Show
        Mark Miller added a comment - Hey Eric, I"m seeing the following tests fail: java.lang.AssertionError: Make sure we did download each file in the original configuration at __randomizedtesting.SeedInfo.seed( [BB2B109CDBABDCE7:9F4DBD9251646A81] :0) at org.junit.Assert.fail(Assert.java:93) at org.junit.Assert.assertTrue(Assert.java:43) at org.apache.solr.cloud.ZkCLITest.testUpConfigLinkConfigClearZk(ZkCLITest.java:182)
        Hide
        Erik Hatcher added a comment -

        I'm not a fan of this change:

        Index: example/solr/collection1/conf/velocity/header.vm
        ===================================================================
        — example/solr/collection1/conf/velocity/header.vm (revision 1433108)
        +++ example/solr/collection1/conf/velocity/header.vm (working copy)
        @@ -1,3 +1,3 @@
        <div id="head">

        • <span ><a href="#url_for_home#if($request.params.get('debugQuery'))?debugQuery=true#end"><img src="#
          Unknown macro: {url_root}

          /img/solr.png" id="logo"/></a></span>
          + <span ><a href="#url_for_home#if($request.params.get('debugQuery'))?debugQuery=true#end"><img src="#

          Unknown macro: {url_for_solr}

          /admin/file?file=/velocity/solr.png&contentType=image/png" id="logo"/></a></span>
          </div>

        This means that solr.png needs to be duplicated into every core's configuration rather than coming from the Solr web app itself. I get this as an example of serving a binary file from ZK, but let's not bake it into the collection1 example like this.

        Show
        Erik Hatcher added a comment - I'm not a fan of this change: Index: example/solr/collection1/conf/velocity/header.vm =================================================================== — example/solr/collection1/conf/velocity/header.vm (revision 1433108) +++ example/solr/collection1/conf/velocity/header.vm (working copy) @@ -1,3 +1,3 @@ <div id="head"> <span ><a href="#url_for_home#if($request.params.get('debugQuery'))?debugQuery=true#end"><img src="# Unknown macro: {url_root} /img/solr.png" id="logo"/></a></span> + <span ><a href="#url_for_home#if($request.params.get('debugQuery'))?debugQuery=true#end"><img src="# Unknown macro: {url_for_solr} /admin/file?file=/velocity/solr.png&contentType=image/png" id="logo"/></a></span> </div> This means that solr.png needs to be duplicated into every core's configuration rather than coming from the Solr web app itself. I get this as an example of serving a binary file from ZK, but let's not bake it into the collection1 example like this.
        Hide
        Steve Rowe added a comment -

        Mark, Erik, Eric: can we push this to 4.2?

        Show
        Steve Rowe added a comment - Mark, Erik, Eric: can we push this to 4.2?
        Hide
        Eric Pugh added a comment -

        This isn't worth delaying a release! I am somewhat struggling to figure out why the test is failing. Going to try on a new code checkout.

        Show
        Eric Pugh added a comment - This isn't worth delaying a release! I am somewhat struggling to figure out why the test is failing. Going to try on a new code checkout.
        Hide
        Mark Miller added a comment -

        Let me know if you make any progress Eric - I'd like this to make 4.2.

        Show
        Mark Miller added a comment - Let me know if you make any progress Eric - I'd like this to make 4.2.
        Hide
        Mark Miller added a comment -

        Ping - still interested in this Eric? I'll get to it eventually anyway, but it may take a little longer.

        Show
        Mark Miller added a comment - Ping - still interested in this Eric? I'll get to it eventually anyway, but it may take a little longer.
        Hide
        Commit Tag Bot added a comment -

        [trunk commit] markrmiller
        http://svn.apache.org/viewvc?view=revision&revision=1481675

        SOLR-4234: Add support for binary files in ZooKeeper.

        Show
        Commit Tag Bot added a comment - [trunk commit] markrmiller http://svn.apache.org/viewvc?view=revision&revision=1481675 SOLR-4234 : Add support for binary files in ZooKeeper.
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] markrmiller
        http://svn.apache.org/viewvc?view=revision&revision=1481676

        SOLR-4234: Add support for binary files in ZooKeeper.

        Show
        Commit Tag Bot added a comment - [branch_4x commit] markrmiller http://svn.apache.org/viewvc?view=revision&revision=1481676 SOLR-4234 : Add support for binary files in ZooKeeper.
        Hide
        Mark Miller added a comment -

        This is done.

        Show
        Mark Miller added a comment - This is done.
        Hide
        Steve Rowe added a comment -

        Bulk close resolved 4.4 issues

        Show
        Steve Rowe added a comment - Bulk close resolved 4.4 issues

          People

          • Assignee:
            Mark Miller
            Reporter:
            Eric Pugh
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development