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

Collection level backup/restore should provide a param for specifying the repository implementation it should use

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.2, 7.0
    • Component/s: None
    • Labels:
      None

      Description

      SOLR-7374 provides BackupRepository interface to enable storing Solr index data to a configured file-system (e.g. HDFS, local file-system etc.). This JIRA is to track the work required to extend this functionality at the collection level.

      1. 7726.log.gz
        134 kB
        Steve Rowe
      2. SOLR-9242_followup.patch
        16 kB
        Varun Thacker
      3. SOLR-9242_followup2.patch
        6 kB
        Varun Thacker
      4. SOLR-9242.patch
        65 kB
        Varun Thacker
      5. SOLR-9242.patch
        64 kB
        Varun Thacker
      6. SOLR-9242.patch
        80 kB
        Hrishikesh Gadre
      7. SOLR-9242.patch
        79 kB
        Hrishikesh Gadre
      8. SOLR-9242.patch
        76 kB
        Hrishikesh Gadre

        Issue Links

          Activity

          Hide
          hgadre Hrishikesh Gadre added a comment -

          Varun Thacker Please find the patch attached. It includes following,

          • Updated the collection level backup/restore implementation to use the BackupRepository interface.
          • Removed the cluster property to define a default backup location. The default will now be associated with the backup repository configuration.
          • Unified the backup/restore API parameter constants in CoreAdminParams class (and removed duplicate declarations in other classes).
          • Added unit test to verify the HDFS integration for collection level backup/restore.
          Show
          hgadre Hrishikesh Gadre added a comment - Varun Thacker Please find the patch attached. It includes following, Updated the collection level backup/restore implementation to use the BackupRepository interface. Removed the cluster property to define a default backup location. The default will now be associated with the backup repository configuration. Unified the backup/restore API parameter constants in CoreAdminParams class (and removed duplicate declarations in other classes). Added unit test to verify the HDFS integration for collection level backup/restore.
          Hide
          varunthacker Varun Thacker added a comment -

          Hi,

          I had a quick peek into the patches and looks good overall! Some nice cleanups as well.

          Removed the cluster property to define a default backup location. The default will now be associated with the backup repository configuration.

          I wanted to get this hashed out before Solr 6.1 came out so that we know whether we want to support this or not. But now that its out and documented I don't think we can drop it easily. Seems like a good way to override location anyways? We'll need to add support for it now

          Show
          varunthacker Varun Thacker added a comment - Hi, I had a quick peek into the patches and looks good overall! Some nice cleanups as well. Removed the cluster property to define a default backup location. The default will now be associated with the backup repository configuration. I wanted to get this hashed out before Solr 6.1 came out so that we know whether we want to support this or not. But now that its out and documented I don't think we can drop it easily. Seems like a good way to override location anyways? We'll need to add support for it now
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Varun Thacker Thanks for the review.

          But now that its out and documented I don't think we can drop it easily. Seems like a good way to override location anyways? We'll need to add support for it now

          Since we allow users to configure multiple repositories in solr.xml, we can not use the current cluster property as is. This is because user may want to specify different location for different file-systems (or repositories). Hence at minimum we need one cluster property per repository configuration (e.g. name could be <repository-name>-location).

          For 6.x releases, we can restrict the users to configure only a single repository at-a-time. This will avoid the problem mentioned above and they can use the current property.
          For 7.x releases, we have multiple options,

          • Support configuring multiple cluster properties one per repository configuration. This will require some changes in CLUSTERPROP API since currently it requires fixed (or well-known) property names. In this case we will remove the current cluster property as well as ability to configure default location via solr.xml
          • Continue using the current mechanism of configuring default location via solr.xml. Just remove the current cluster property

          What do you think?

          Show
          hgadre Hrishikesh Gadre added a comment - Varun Thacker Thanks for the review. But now that its out and documented I don't think we can drop it easily. Seems like a good way to override location anyways? We'll need to add support for it now Since we allow users to configure multiple repositories in solr.xml, we can not use the current cluster property as is. This is because user may want to specify different location for different file-systems (or repositories). Hence at minimum we need one cluster property per repository configuration (e.g. name could be <repository-name>-location). For 6.x releases, we can restrict the users to configure only a single repository at-a-time. This will avoid the problem mentioned above and they can use the current property. For 7.x releases, we have multiple options, Support configuring multiple cluster properties one per repository configuration. This will require some changes in CLUSTERPROP API since currently it requires fixed (or well-known) property names. In this case we will remove the current cluster property as well as ability to configure default location via solr.xml Continue using the current mechanism of configuring default location via solr.xml. Just remove the current cluster property What do you think?
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Varun Thacker Mark Miller Any thoughts on this?

          Show
          hgadre Hrishikesh Gadre added a comment - Varun Thacker Mark Miller Any thoughts on this?
          Hide
          varunthacker Varun Thacker added a comment -

          Hi Hrishikesh,

          we can restrict the users to configure only a single repository at-a-time. This will avoid the problem mentioned above and they can use the current property.

          Personally I don't like the idea of limiting our users to one repo in all of the 6.x line.

          Let's say we follow this order:
          1. If "location" param was provided as a query param use that
          2. Else if the "repository" in the solr.xml has a "location" param use that.
          3. If the "repository" specified doesn't specify a "location" param then see if it's specified via the cluster property API . The code will throw an error if the location was bogus or was with not for this repo. It has to fail as the repository will fail to read / write to that location.

          I thought about the "repoName:/path" syntax idea that you proposed . Seems to me that we want to do all of this because solr.xml doesn't have an API to update it . We have to hand edit the file and upload to ZK at the very least.

          I think let's not complicate it any further? Keep the location cluster prop for now the way it is and support it.

          We can work towards adding API support to solr.xml and then get rid the "location" cluster prop or the entire cluster prop API in the future.

          Show
          varunthacker Varun Thacker added a comment - Hi Hrishikesh, we can restrict the users to configure only a single repository at-a-time. This will avoid the problem mentioned above and they can use the current property. Personally I don't like the idea of limiting our users to one repo in all of the 6.x line. Let's say we follow this order: 1. If "location" param was provided as a query param use that 2. Else if the "repository" in the solr.xml has a "location" param use that. 3. If the "repository" specified doesn't specify a "location" param then see if it's specified via the cluster property API . The code will throw an error if the location was bogus or was with not for this repo. It has to fail as the repository will fail to read / write to that location. I thought about the "repoName:/path" syntax idea that you proposed . Seems to me that we want to do all of this because solr.xml doesn't have an API to update it . We have to hand edit the file and upload to ZK at the very least. I think let's not complicate it any further? Keep the location cluster prop for now the way it is and support it. We can work towards adding API support to solr.xml and then get rid the "location" cluster prop or the entire cluster prop API in the future.
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Varun Thacker Thanks for the feedback. I have incorporated these review comments in this patch. Please take a look and let me know.

          Show
          hgadre Hrishikesh Gadre added a comment - Varun Thacker Thanks for the feedback. I have incorporated these review comments in this patch. Please take a look and let me know.
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Varun Thacker Filed SOLR-9268 to track API support for solr.xml configuration.

          Show
          hgadre Hrishikesh Gadre added a comment - Varun Thacker Filed SOLR-9268 to track API support for solr.xml configuration.
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Varun Thacker Fixed a bug in the earlier patch.

          Show
          hgadre Hrishikesh Gadre added a comment - Varun Thacker Fixed a bug in the earlier patch.
          Hide
          varunthacker Varun Thacker added a comment -

          Hi Hrishikesh,

          Thanks for the patch! I'll have a look at this over the weekend.

          Show
          varunthacker Varun Thacker added a comment - Hi Hrishikesh, Thanks for the patch! I'll have a look at this over the weekend.
          Hide
          varunthacker Varun Thacker added a comment -

          Thanks Hrishikesh for the patch!

          I've attached a new patch with some minor changes listed below. Let me know how it looks.

          OverseerCollectionMessageHandler
          1. In processBackupAction remove unused variable propertiesPath
          2. In processBackupAction/processRestoreAction remove the "location" checks. Error handling and validation has ready been done at the CollectionsHandler
          3. In processRestoreAction removed unused variable backupZkPath

          CoreContainer
          1. One minor formatting change

          BackupManager
          1. Remove unused import

          CollectionsHandler
          1. Used LOCATION contant at a couple of missing places.

          AbstractCloudBackupRestoreTestCase

          1. In testInvalidPath where we set cluster property , added asset that the property has been set successfully

          TestHdfsCloudBackupRestore

          1. This prop solr.hdfs.confdir in the solr.xml file never seemed to be getting used? I removed it and the tests pass. Do we need this?

          CollectionAdminRequest

          1. Changed setRepository(Optional<String> repository) to setRepository(String repository . Seems cleaner from an API perspective given it's a setter.
          2. Changed the variable name from repository to repositoryName
          3. Made necessary changes to AbstractCloudBackupRestoreTestCase to fix the compile errors because of the first two changes.

          ReplicationHandler

          1. Minor Formatting Changes

          TestBackupRepositoryFactory

          1. Reverted the change of using the "location" string VS constant. If by change the constant name, Strings in tests will help us catch it.

          Show
          varunthacker Varun Thacker added a comment - Thanks Hrishikesh for the patch! I've attached a new patch with some minor changes listed below. Let me know how it looks. OverseerCollectionMessageHandler 1. In processBackupAction remove unused variable propertiesPath 2. In processBackupAction/processRestoreAction remove the "location" checks. Error handling and validation has ready been done at the CollectionsHandler 3. In processRestoreAction removed unused variable backupZkPath CoreContainer 1. One minor formatting change BackupManager 1. Remove unused import CollectionsHandler 1. Used LOCATION contant at a couple of missing places. AbstractCloudBackupRestoreTestCase 1. In testInvalidPath where we set cluster property , added asset that the property has been set successfully TestHdfsCloudBackupRestore 1. This prop solr.hdfs.confdir in the solr.xml file never seemed to be getting used? I removed it and the tests pass. Do we need this? CollectionAdminRequest 1. Changed setRepository(Optional<String> repository) to setRepository(String repository . Seems cleaner from an API perspective given it's a setter. 2. Changed the variable name from repository to repositoryName 3. Made necessary changes to AbstractCloudBackupRestoreTestCase to fix the compile errors because of the first two changes. ReplicationHandler 1. Minor Formatting Changes TestBackupRepositoryFactory 1. Reverted the change of using the "location" string VS constant. If by change the constant name, Strings in tests will help us catch it.
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Varun Thacker Thanks for the review comments. The updated patch looks good mostly. Only couple of minor comments,

          This prop solr.hdfs.confdir in the solr.xml file never seemed to be getting used? I removed it and the tests pass. Do we need this?

          The HdfsBackupRepository uses HdfsDirectory internally. This optional property is being used by the HdfsDirectoryFactory. My guess is that in the test environment Hadoop conf directory is not created/initialized. I don't think there is any harm in keeping this configuration option.

          Changed setRepository(Optional<String> repository) to setRepository(String repository . Seems cleaner from an API perspective given it's a setter.

          I am OK with changing the setRepository parameter from Optional<String> to String. But it looks like you have changed the attribute type as well. Since we are using Java 8 now, we should use Optional type to clearly specify optional attributes. This helps to improve the readability of the code. Here is a good article about the Java 8 Optional type. http://blog.codefx.org/techniques/intention-revealing-code-java-8-optional/

          How about following?

          • The parameter type of setter should be String. The setter method should initialize the attribute based on its nullability.
          • The type of attribute as well as the getter should be Optional<String>. This clearly indicates that the attribute is optional (without having to read the code comment in the CollectionAdminRequest class).
          • In the getParams method - replace the null check with isPresent() method call.
          Show
          hgadre Hrishikesh Gadre added a comment - Varun Thacker Thanks for the review comments. The updated patch looks good mostly. Only couple of minor comments, This prop solr.hdfs.confdir in the solr.xml file never seemed to be getting used? I removed it and the tests pass. Do we need this? The HdfsBackupRepository uses HdfsDirectory internally. This optional property is being used by the HdfsDirectoryFactory. My guess is that in the test environment Hadoop conf directory is not created/initialized. I don't think there is any harm in keeping this configuration option. Changed setRepository(Optional<String> repository) to setRepository(String repository . Seems cleaner from an API perspective given it's a setter. I am OK with changing the setRepository parameter from Optional<String> to String. But it looks like you have changed the attribute type as well. Since we are using Java 8 now, we should use Optional type to clearly specify optional attributes. This helps to improve the readability of the code. Here is a good article about the Java 8 Optional type. http://blog.codefx.org/techniques/intention-revealing-code-java-8-optional/ How about following? The parameter type of setter should be String. The setter method should initialize the attribute based on its nullability. The type of attribute as well as the getter should be Optional<String>. This clearly indicates that the attribute is optional (without having to read the code comment in the CollectionAdminRequest class). In the getParams method - replace the null check with isPresent() method call.
          Hide
          varunthacker Varun Thacker added a comment -

          Makes sense! I've updated the patch.

          Running all the tests before committing it.

          Show
          varunthacker Varun Thacker added a comment - Makes sense! I've updated the patch. Running all the tests before committing it.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit bfe5c5ae499499d4198caa71442eb3e4eba45237 in lucene-solr's branch refs/heads/master from Varun Thacker
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bfe5c5a ]

          SOLR-9242: Collection backup/restore to provide a param for specifying underlying storage repository to use

          Show
          jira-bot ASF subversion and git services added a comment - Commit bfe5c5ae499499d4198caa71442eb3e4eba45237 in lucene-solr's branch refs/heads/master from Varun Thacker [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bfe5c5a ] SOLR-9242 : Collection backup/restore to provide a param for specifying underlying storage repository to use
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 413ea476700429bd39c659cdd0bc6682263b0545 in lucene-solr's branch refs/heads/branch_6x from Varun Thacker
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=413ea47 ]

          SOLR-9242: Collection backup/restore to provide a param for specifying underlying storage repository to use

          Show
          jira-bot ASF subversion and git services added a comment - Commit 413ea476700429bd39c659cdd0bc6682263b0545 in lucene-solr's branch refs/heads/branch_6x from Varun Thacker [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=413ea47 ] SOLR-9242 : Collection backup/restore to provide a param for specifying underlying storage repository to use
          Hide
          varunthacker Varun Thacker added a comment -

          Thanks Hrishikesh! Committed this

          Show
          varunthacker Varun Thacker added a comment - Thanks Hrishikesh! Committed this
          Hide
          varunthacker Varun Thacker added a comment -

          We have a test failure for windows :

          Log excerpt :

             [junit4]   2> Caused by: java.nio.file.InvalidPathException: Illegal char <:> at index 2: /C:/Users/jenkins/workspace/Lucene-Solr-6.x-Windows/solr/build/solr-core/test/J1/temp/solr.cloud.TestLocalFSCloudBackupRestore_168D4B6DEE507089-001/tempDir-002/mytestbackup
             [junit4]   2> 	at sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
             [junit4]   2> 	at sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
             [junit4]   2> 	at sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
             [junit4]   2> 	at sun.nio.fs.WindowsPath.parse(WindowsPath.java:94)
             [junit4]   2> 	at sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:255)
             [junit4]   2> 	at java.nio.file.Paths.get(Paths.java:84)
             [junit4]   2> 	at org.apache.solr.core.backup.repository.LocalFileSystemRepository.createURI(LocalFileSystemRepository.java:62)
             [junit4]   2> 	at org.apache.solr.handler.SnapShooter.initialize(SnapShooter.java:85)
             [junit4]   2> 	at org.apache.solr.handler.SnapShooter.<init>(SnapShooter.java:79)
             [junit4]   2> 	at org.apache.solr.handler.admin.CoreAdminOperation$19.call(CoreAdminOperation.java:873)
             [junit4]   2> 	... 30 more
          

          Jenkins failure link : http://jenkins.thetaphi.de/job/Lucene-Solr-6.x-Windows/305/

          Show
          varunthacker Varun Thacker added a comment - We have a test failure for windows : Log excerpt : [junit4] 2> Caused by: java.nio.file.InvalidPathException: Illegal char <:> at index 2: /C:/Users/jenkins/workspace/Lucene-Solr-6.x-Windows/solr/build/solr-core/test/J1/temp/solr.cloud.TestLocalFSCloudBackupRestore_168D4B6DEE507089-001/tempDir-002/mytestbackup [junit4] 2> at sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182) [junit4] 2> at sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153) [junit4] 2> at sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77) [junit4] 2> at sun.nio.fs.WindowsPath.parse(WindowsPath.java:94) [junit4] 2> at sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:255) [junit4] 2> at java.nio.file.Paths.get(Paths.java:84) [junit4] 2> at org.apache.solr.core.backup.repository.LocalFileSystemRepository.createURI(LocalFileSystemRepository.java:62) [junit4] 2> at org.apache.solr.handler.SnapShooter.initialize(SnapShooter.java:85) [junit4] 2> at org.apache.solr.handler.SnapShooter.<init>(SnapShooter.java:79) [junit4] 2> at org.apache.solr.handler.admin.CoreAdminOperation$19.call(CoreAdminOperation.java:873) [junit4] 2> ... 30 more Jenkins failure link : http://jenkins.thetaphi.de/job/Lucene-Solr-6.x-Windows/305/
          Hide
          dsmiley David Smiley added a comment -

          BTW we should be putting our ASF source headers at the very top. I notice these files put them in other places, and in at least one place moved existing ones to another ordering. I wish precommit with complain about this.

          Show
          dsmiley David Smiley added a comment - BTW we should be putting our ASF source headers at the very top. I notice these files put them in other places, and in at least one place moved existing ones to another ordering. I wish precommit with complain about this.
          Hide
          varunthacker Varun Thacker added a comment -

          There's another type of test failure : https://builds.apache.org/job/Lucene-Solr-Tests-master/1270/console

          In this case the ZK watch for updating the clusterprop occurs after the backup command has been invoked . Hence it doesn't find the value set in the cluster-prop api

          [junit4]   2> 819240 INFO  (qtp102693249-7831) [n:127.0.0.1:52710_solr    ] o.a.s.h.a.CollectionsHandler Invoked Collection Action :clusterprop with params val=/location/does/not/exist&name=location&action=CLUSTERPROP&wt=javabin&version=2 and sendToOCPQueue=true
             [junit4]   2> 819241 INFO  (qtp102693249-7831) [n:127.0.0.1:52710_solr    ] o.a.s.s.HttpSolrCall [admin] webapp=null path=/admin/collections params={val=/location/does/not/exist&name=location&action=CLUSTERPROP&wt=javabin&version=2} status=0 QTime=0
             [junit4]   2> 819242 INFO  (qtp102693249-7825) [n:127.0.0.1:52710_solr    ] o.a.s.h.a.CollectionsHandler Invoked Collection Action :backup with params name=invalidbackuprequest&action=BACKUP&collection=backuprestore&wt=javabin&version=2 and sendToOCPQueue=true
             [junit4]   2> 819242 INFO  (zkCallback-1166-thread-1) [    ] o.a.s.c.c.ZkStateReader Loaded cluster properties: {location=/location/does/not/exist}
             [junit4]   2> 819242 INFO  (zkCallback-1160-thread-2-processing-n:127.0.0.1:41141_solr) [n:127.0.0.1:41141_solr    ] o.a.s.c.c.ZkStateReader Loaded cluster properties: {location=/location/does/not/exist}
             [junit4]   2> 819242 ERROR (qtp102693249-7825) [n:127.0.0.1:52710_solr    ] o.a.s.h.RequestHandlerBase org.apache.solr.common.SolrException: 'location' is not specified as a query parameter or as a default repository property or as a cluster property.
             [junit4]   2> 	at org.apache.solr.handler.admin.CollectionsHandler$CollectionOperation$28.call(CollectionsHandler.java:822)
             [junit4]   2> 	at org.apache.solr.handler.admin.CollectionsHandler.handleRequestBody(CollectionsHandler.java:207)
             [junit4]   2> 	at org.apache.solr.handler.RequestHandlerBase.handleRequest(RequestHandlerBase.java:154)
             [junit4]   2> 	at org.apache.solr.servlet.HttpSolrCall.handleAdminRequest(HttpSolrCall.java:659)
             [junit4]   2> 	at org.apache.solr.servlet.HttpSolrCall.call(HttpSolrCall.java:441)
             [junit4]   2> 	at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:257)
             [junit4]   2> 	at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:208)
             [junit4]   2> 	at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1676)
             [junit4]   2> 	at org.apache.solr.client.solrj.embedded.JettySolrRunner$DebugFilter.doFilter(JettySolrRunner.java:111)
             [junit4]   2> 	at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1676)
             [junit4]   2> 	at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:581)
             [junit4]   2> 	at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:224)
             [junit4]   2> 	at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1160)
             [junit4]   2> 	at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:511)
             [junit4]   2> 	at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:185)
             [junit4]   2> 	at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1092)
             [junit4]   2> 	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141)
             [junit4]   2> 	at org.eclipse.jetty.server.handler.gzip.GzipHandler.handle(GzipHandler.java:462)
             [junit4]   2> 	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:134)
             [junit4]   2> 	at org.eclipse.jetty.server.Server.handle(Server.java:518)
             [junit4]   2> 	at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:308)
             [junit4]   2> 	at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:244)
             [junit4]   2> 	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:273)
             [junit4]   2> 	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:95)
             [junit4]   2> 	at org.eclipse.jetty.io.SelectChannelEndPoint$2.run(SelectChannelEndPoint.java:93)
             [junit4]   2> 	at org.eclipse.jetty.util.thread.strategy.ExecuteProduceConsume.produceAndRun(ExecuteProduceConsume.java:246)
             [junit4]   2> 	at org.eclipse.jetty.util.thread.strategy.ExecuteProduceConsume.run(ExecuteProduceConsume.java:156)
             [junit4]   2> 	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:654)
             [junit4]   2> 	at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:572)
             [junit4]   2> 	at java.lang.Thread.run(Thread.java:745)
             [junit4]   2> 
             [junit4]   2> 819242 INFO  (qtp102693249-7825) [n:127.0.0.1:52710_solr    ] o.a.s.s.HttpSolrCall [admin] webapp=null path=/admin/collections params={name=invalidbackuprequest&action=BACKUP&collection=backuprestore&wt=javabin&version=2} status=400 QTime=0
             [junit4]   2> 819242 INFO  (zkCallback-1159-thread-1-processing-n:127.0.0.1:52710_solr) [n:127.0.0.1:52710_solr    ] o.a.s.c.c.ZkStateReader Loaded cluster properties: {location=/location/does/not/exist}
          

          Line 819242 should occur before 819242

          Show
          varunthacker Varun Thacker added a comment - There's another type of test failure : https://builds.apache.org/job/Lucene-Solr-Tests-master/1270/console In this case the ZK watch for updating the clusterprop occurs after the backup command has been invoked . Hence it doesn't find the value set in the cluster-prop api [junit4] 2> 819240 INFO (qtp102693249-7831) [n:127.0.0.1:52710_solr ] o.a.s.h.a.CollectionsHandler Invoked Collection Action :clusterprop with params val=/location/does/not/exist&name=location&action=CLUSTERPROP&wt=javabin&version=2 and sendToOCPQueue= true [junit4] 2> 819241 INFO (qtp102693249-7831) [n:127.0.0.1:52710_solr ] o.a.s.s.HttpSolrCall [admin] webapp= null path=/admin/collections params={val=/location/does/not/exist&name=location&action=CLUSTERPROP&wt=javabin&version=2} status=0 QTime=0 [junit4] 2> 819242 INFO (qtp102693249-7825) [n:127.0.0.1:52710_solr ] o.a.s.h.a.CollectionsHandler Invoked Collection Action :backup with params name=invalidbackuprequest&action=BACKUP&collection=backuprestore&wt=javabin&version=2 and sendToOCPQueue= true [junit4] 2> 819242 INFO (zkCallback-1166-thread-1) [ ] o.a.s.c.c.ZkStateReader Loaded cluster properties: {location=/location/does/not/exist} [junit4] 2> 819242 INFO (zkCallback-1160-thread-2-processing-n:127.0.0.1:41141_solr) [n:127.0.0.1:41141_solr ] o.a.s.c.c.ZkStateReader Loaded cluster properties: {location=/location/does/not/exist} [junit4] 2> 819242 ERROR (qtp102693249-7825) [n:127.0.0.1:52710_solr ] o.a.s.h.RequestHandlerBase org.apache.solr.common.SolrException: 'location' is not specified as a query parameter or as a default repository property or as a cluster property. [junit4] 2> at org.apache.solr.handler.admin.CollectionsHandler$CollectionOperation$28.call(CollectionsHandler.java:822) [junit4] 2> at org.apache.solr.handler.admin.CollectionsHandler.handleRequestBody(CollectionsHandler.java:207) [junit4] 2> at org.apache.solr.handler.RequestHandlerBase.handleRequest(RequestHandlerBase.java:154) [junit4] 2> at org.apache.solr.servlet.HttpSolrCall.handleAdminRequest(HttpSolrCall.java:659) [junit4] 2> at org.apache.solr.servlet.HttpSolrCall.call(HttpSolrCall.java:441) [junit4] 2> at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:257) [junit4] 2> at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:208) [junit4] 2> at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1676) [junit4] 2> at org.apache.solr.client.solrj.embedded.JettySolrRunner$DebugFilter.doFilter(JettySolrRunner.java:111) [junit4] 2> at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1676) [junit4] 2> at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:581) [junit4] 2> at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:224) [junit4] 2> at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1160) [junit4] 2> at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:511) [junit4] 2> at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:185) [junit4] 2> at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1092) [junit4] 2> at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141) [junit4] 2> at org.eclipse.jetty.server.handler.gzip.GzipHandler.handle(GzipHandler.java:462) [junit4] 2> at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:134) [junit4] 2> at org.eclipse.jetty.server.Server.handle(Server.java:518) [junit4] 2> at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:308) [junit4] 2> at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:244) [junit4] 2> at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:273) [junit4] 2> at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:95) [junit4] 2> at org.eclipse.jetty.io.SelectChannelEndPoint$2.run(SelectChannelEndPoint.java:93) [junit4] 2> at org.eclipse.jetty.util.thread.strategy.ExecuteProduceConsume.produceAndRun(ExecuteProduceConsume.java:246) [junit4] 2> at org.eclipse.jetty.util.thread.strategy.ExecuteProduceConsume.run(ExecuteProduceConsume.java:156) [junit4] 2> at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:654) [junit4] 2> at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:572) [junit4] 2> at java.lang. Thread .run( Thread .java:745) [junit4] 2> [junit4] 2> 819242 INFO (qtp102693249-7825) [n:127.0.0.1:52710_solr ] o.a.s.s.HttpSolrCall [admin] webapp= null path=/admin/collections params={name=invalidbackuprequest&action=BACKUP&collection=backuprestore&wt=javabin&version=2} status=400 QTime=0 [junit4] 2> 819242 INFO (zkCallback-1159-thread-1-processing-n:127.0.0.1:52710_solr) [n:127.0.0.1:52710_solr ] o.a.s.c.c.ZkStateReader Loaded cluster properties: {location=/location/does/not/exist} Line 819242 should occur before 819242
          Hide
          varunthacker Varun Thacker added a comment -

          I'm looking into the windows path failure as well

          Show
          varunthacker Varun Thacker added a comment - Moved ASF source headers at the very top ForceUpdate cluster props to make sure we are reading the latest value. This tackles Jenkins failures like https://builds.apache.org/job/Lucene-Solr-Tests-master/1270/ I'm looking into the windows path failure as well
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit eefdc62c997f7936b6db203111d8149dc934b243 in lucene-solr's branch refs/heads/master from Varun Thacker
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=eefdc62 ]

          SOLR-9242: Move license headers to the top + force refresh cluster propery before reading the 'location' param

          Show
          jira-bot ASF subversion and git services added a comment - Commit eefdc62c997f7936b6db203111d8149dc934b243 in lucene-solr's branch refs/heads/master from Varun Thacker [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=eefdc62 ] SOLR-9242 : Move license headers to the top + force refresh cluster propery before reading the 'location' param
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1dfc637f2ca10242afeb12ba1ad35357ed611029 in lucene-solr's branch refs/heads/branch_6x from Varun Thacker
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1dfc637 ]

          SOLR-9242: Move license headers to the top + force refresh cluster propery before reading the 'location' param

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1dfc637f2ca10242afeb12ba1ad35357ed611029 in lucene-solr's branch refs/heads/branch_6x from Varun Thacker [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1dfc637 ] SOLR-9242 : Move license headers to the top + force refresh cluster propery before reading the 'location' param
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Varun Thacker Sorry for late reply. It seems like the changes committed regarding the force loading the cluster properties are in direct contradiction with SOLR-9106. Since this is unrelated to backup/restore, I think we should have handled it separately.

          Regarding windows failure -
          I don't have the windows setup. But let me figure something out and provide a fix...

          Show
          hgadre Hrishikesh Gadre added a comment - Varun Thacker Sorry for late reply. It seems like the changes committed regarding the force loading the cluster properties are in direct contradiction with SOLR-9106 . Since this is unrelated to backup/restore, I think we should have handled it separately. Regarding windows failure - I don't have the windows setup. But let me figure something out and provide a fix...
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Found one more unit test failure on Windows. Investigating...

          http://jenkins.thetaphi.de/job/Lucene-Solr-master-Windows/5996/

          Show
          hgadre Hrishikesh Gadre added a comment - Found one more unit test failure on Windows. Investigating... http://jenkins.thetaphi.de/job/Lucene-Solr-master-Windows/5996/
          Hide
          varunthacker Varun Thacker added a comment -

          Thanks for Investigating!

          I've been swamped with work otherwise I would have got to this sooner.

          Show
          varunthacker Varun Thacker added a comment - Thanks for Investigating! I've been swamped with work otherwise I would have got to this sooner.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 219406d912e027195145de2e77f35f41a6116c6d in lucene-solr's branch refs/heads/master from Varun Thacker
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=219406d ]

          SOLR-9242: Disabling TestLocalFSCloudBackupRestore on Windows till we fix it

          Show
          jira-bot ASF subversion and git services added a comment - Commit 219406d912e027195145de2e77f35f41a6116c6d in lucene-solr's branch refs/heads/master from Varun Thacker [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=219406d ] SOLR-9242 : Disabling TestLocalFSCloudBackupRestore on Windows till we fix it
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 12449282624a2342dde6a51a90872b104a23560a in lucene-solr's branch refs/heads/branch_6x from Varun Thacker
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1244928 ]

          SOLR-9242: Disabling TestLocalFSCloudBackupRestore on Windows till we fix it

          Show
          jira-bot ASF subversion and git services added a comment - Commit 12449282624a2342dde6a51a90872b104a23560a in lucene-solr's branch refs/heads/branch_6x from Varun Thacker [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1244928 ] SOLR-9242 : Disabling TestLocalFSCloudBackupRestore on Windows till we fix it
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user hgadre opened a pull request:

          https://github.com/apache/lucene-solr/pull/62

          SOLR-9242 Fix unit test failure on Windows platform.

          The root cause of the failure is the usage of URI::getPath() method in the backup/restore functionality (e.g. in BackupManager::downloadFromZK OR in the OverseerCollectionMessageHandler::processBackupAction methods). On the Windows platform, usage of URI.getPath() returns an invalid path string (e.g. URI file:///C:/lucene-solr/solr
          returns /C:/lucene-solr/solr as the result of getPath() method).

          Refer to following discussion for more details,
          http://stackoverflow.com/questions/9834776/java-nio-file-path-issue

          Since the caller may have used this method to generate the string representation for the pathComponents, we implement a work-around specifically for Windows platform to remove the leading '/' character.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/hgadre/lucene-solr SOLR-9242_fix

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/lucene-solr/pull/62.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #62



          Show
          githubbot ASF GitHub Bot added a comment - GitHub user hgadre opened a pull request: https://github.com/apache/lucene-solr/pull/62 SOLR-9242 Fix unit test failure on Windows platform. The root cause of the failure is the usage of URI::getPath() method in the backup/restore functionality (e.g. in BackupManager::downloadFromZK OR in the OverseerCollectionMessageHandler::processBackupAction methods). On the Windows platform, usage of URI.getPath() returns an invalid path string (e.g. URI file:///C:/lucene-solr/solr returns /C:/lucene-solr/solr as the result of getPath() method). Refer to following discussion for more details, http://stackoverflow.com/questions/9834776/java-nio-file-path-issue Since the caller may have used this method to generate the string representation for the pathComponents, we implement a work-around specifically for Windows platform to remove the leading '/' character. You can merge this pull request into a Git repository by running: $ git pull https://github.com/hgadre/lucene-solr SOLR-9242 _fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/lucene-solr/pull/62.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #62
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hgadre commented on the issue:

          https://github.com/apache/lucene-solr/pull/62

          @vthacker could you please take a look?

          Show
          githubbot ASF GitHub Bot added a comment - Github user hgadre commented on the issue: https://github.com/apache/lucene-solr/pull/62 @vthacker could you please take a look?
          Hide
          varunthacker Varun Thacker added a comment -

          Hi Hrishikesh,

          Thanks for the patch! My research had also shown this same solution. Maybe there isn't a better solution. So lets use this . Maybe if someone has a better solution we can move to that in the future.

          Additionally made another small change based on the latest comments from SOLR-9106.

          Show
          varunthacker Varun Thacker added a comment - Hi Hrishikesh, Thanks for the patch! My research had also shown this same solution. Maybe there isn't a better solution. So lets use this . Maybe if someone has a better solution we can move to that in the future. Additionally made another small change based on the latest comments from SOLR-9106 .
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Varun Thacker Thanks for the review and updated patch. Looks good to me!

          Show
          hgadre Hrishikesh Gadre added a comment - Varun Thacker Thanks for the review and updated patch. Looks good to me!
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d5a7ca79f3ac88d4de54c013eb6b29f72e52c907 in lucene-solr's branch refs/heads/master from Varun Thacker
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d5a7ca7 ]

          SOLR-9242: fix windows path issue + load live cluster properties. This closes #62

          Show
          jira-bot ASF subversion and git services added a comment - Commit d5a7ca79f3ac88d4de54c013eb6b29f72e52c907 in lucene-solr's branch refs/heads/master from Varun Thacker [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d5a7ca7 ] SOLR-9242 : fix windows path issue + load live cluster properties. This closes #62
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/lucene-solr/pull/62

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/lucene-solr/pull/62
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 9c37aaabe4e1b3f66cd0034e2e3fe0399e82fffd in lucene-solr's branch refs/heads/branch_6x from Varun Thacker
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9c37aaa ]

          SOLR-9242: fix windows path issue + load live cluster properties. This closes #62

          Show
          jira-bot ASF subversion and git services added a comment - Commit 9c37aaabe4e1b3f66cd0034e2e3fe0399e82fffd in lucene-solr's branch refs/heads/branch_6x from Varun Thacker [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9c37aaa ] SOLR-9242 : fix windows path issue + load live cluster properties. This closes #62
          Hide
          steve_rowe Steve Rowe added a comment -

          My Jenkins found a TestHdfsCloudBackupRestore failure that reproduces for me on master:

             [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestHdfsCloudBackupRestore -Dtests.method=test -Dtests.seed=9470C0688BFAF297 -Dtests.slow=true -Dtests.linedocsfile=/home/jenkins/lucene-data/enwiki.random.lines.txt -Dtests.locale=es-MX -Dtests.timezone=HST -Dtests.asserts=true -Dtests.file.encoding=UTF-8
             [junit4] ERROR   38.6s J5  | TestHdfsCloudBackupRestore.test <<<
             [junit4]    > Throwable #1: org.apache.solr.client.solrj.impl.HttpSolrClient$RemoteSolrException: Error from server at http://127.0.0.1:37868/solr: Collection 'hdfsbackuprestore_restored' exists, no action taken.
             [junit4]    > 	at __randomizedtesting.SeedInfo.seed([9470C0688BFAF297:1C24FFB225069F6F]:0)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpSolrClient.executeMethod(HttpSolrClient.java:608)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:261)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:250)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.LBHttpSolrClient.doRequest(LBHttpSolrClient.java:413)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.LBHttpSolrClient.request(LBHttpSolrClient.java:366)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.CloudSolrClient.sendRequest(CloudSolrClient.java:1291)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.CloudSolrClient.requestWithRetryOnStaleState(CloudSolrClient.java:1061)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.CloudSolrClient.request(CloudSolrClient.java:997)
             [junit4]    > 	at org.apache.solr.client.solrj.SolrRequest.process(SolrRequest.java:149)
             [junit4]    > 	at org.apache.solr.client.solrj.SolrRequest.process(SolrRequest.java:166)
             [junit4]    > 	at org.apache.solr.cloud.AbstractCloudBackupRestoreTestCase.testBackupAndRestore(AbstractCloudBackupRestoreTestCase.java:232)
             [junit4]    > 	at org.apache.solr.cloud.AbstractCloudBackupRestoreTestCase.test(AbstractCloudBackupRestoreTestCase.java:126)
             [junit4]    > 	at java.lang.Thread.run(Thread.java:745)
          
          Show
          steve_rowe Steve Rowe added a comment - My Jenkins found a TestHdfsCloudBackupRestore failure that reproduces for me on master: [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestHdfsCloudBackupRestore -Dtests.method=test -Dtests.seed=9470C0688BFAF297 -Dtests.slow=true -Dtests.linedocsfile=/home/jenkins/lucene-data/enwiki.random.lines.txt -Dtests.locale=es-MX -Dtests.timezone=HST -Dtests.asserts=true -Dtests.file.encoding=UTF-8 [junit4] ERROR 38.6s J5 | TestHdfsCloudBackupRestore.test <<< [junit4] > Throwable #1: org.apache.solr.client.solrj.impl.HttpSolrClient$RemoteSolrException: Error from server at http://127.0.0.1:37868/solr: Collection 'hdfsbackuprestore_restored' exists, no action taken. [junit4] > at __randomizedtesting.SeedInfo.seed([9470C0688BFAF297:1C24FFB225069F6F]:0) [junit4] > at org.apache.solr.client.solrj.impl.HttpSolrClient.executeMethod(HttpSolrClient.java:608) [junit4] > at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:261) [junit4] > at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:250) [junit4] > at org.apache.solr.client.solrj.impl.LBHttpSolrClient.doRequest(LBHttpSolrClient.java:413) [junit4] > at org.apache.solr.client.solrj.impl.LBHttpSolrClient.request(LBHttpSolrClient.java:366) [junit4] > at org.apache.solr.client.solrj.impl.CloudSolrClient.sendRequest(CloudSolrClient.java:1291) [junit4] > at org.apache.solr.client.solrj.impl.CloudSolrClient.requestWithRetryOnStaleState(CloudSolrClient.java:1061) [junit4] > at org.apache.solr.client.solrj.impl.CloudSolrClient.request(CloudSolrClient.java:997) [junit4] > at org.apache.solr.client.solrj.SolrRequest.process(SolrRequest.java:149) [junit4] > at org.apache.solr.client.solrj.SolrRequest.process(SolrRequest.java:166) [junit4] > at org.apache.solr.cloud.AbstractCloudBackupRestoreTestCase.testBackupAndRestore(AbstractCloudBackupRestoreTestCase.java:232) [junit4] > at org.apache.solr.cloud.AbstractCloudBackupRestoreTestCase.test(AbstractCloudBackupRestoreTestCase.java:126) [junit4] > at java.lang.Thread.run(Thread.java:745)
          Hide
          steve_rowe Steve Rowe added a comment -

          Compressed log for the Jenkins failure mentioned in my previous post.

          Show
          steve_rowe Steve Rowe added a comment - Compressed log for the Jenkins failure mentioned in my previous post.
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Steve Rowe Thanks for the update. Let me take a look and get back to you.

          Show
          hgadre Hrishikesh Gadre added a comment - Steve Rowe Thanks for the update. Let me take a look and get back to you.
          Hide
          varunthacker Varun Thacker added a comment -

          hdfsbackuprestore_shard1_0_replica0 - doesn't sound right. We should create another issue to make sure its atleast replica1

          Secondly I see the operation is being retried on failure. That doesn't look good.

          I'll try looking at the main issue as well

          Show
          varunthacker Varun Thacker added a comment - hdfsbackuprestore_shard1_0_replica0 - doesn't sound right. We should create another issue to make sure its atleast replica1 Secondly I see the operation is being retried on failure. That doesn't look good. I'll try looking at the main issue as well
          Hide
          thetaphi Uwe Schindler added a comment - - edited

          Hi,

          can we fix this please in a correct way? The Stackoverflow issue is IMHO showing incompetence of the typical "Stackoverflow" user. URI.getPath() returns the URI path and never ever a filesystem path. The fix is also incorrect. It also breaks if you have whitespace in your path (on non-windows)!!! This explains now why this test always fails on my system!

          To convert an URI to a Path use Paths.get(URI), and not URI.getPath().

          Show
          thetaphi Uwe Schindler added a comment - - edited Hi, can we fix this please in a correct way? The Stackoverflow issue is IMHO showing incompetence of the typical "Stackoverflow" user. URI.getPath() returns the URI path and never ever a filesystem path. The fix is also incorrect. It also breaks if you have whitespace in your path (on non-windows)!!! This explains now why this test always fails on my system! To convert an URI to a Path use Paths.get(URI), and not URI.getPath().
          Hide
          thetaphi Uwe Schindler added a comment -

          I just checked the code: This is a major desaster. The method URI#getPath() and URL.getPath() was already proposed to be put on the forbidden-apis list. I think it's time to do this. This is not acceptable, we should fix asap. To me that would have been also a reason to -1 the release of Solr 5.2!

          Show
          thetaphi Uwe Schindler added a comment - I just checked the code: This is a major desaster. The method URI#getPath() and URL.getPath() was already proposed to be put on the forbidden-apis list. I think it's time to do this. This is not acceptable, we should fix asap. To me that would have been also a reason to -1 the release of Solr 5.2!
          Hide
          varunthacker Varun Thacker added a comment -

          Hi Uwe,

          I created SOLR-9444 to track this. I'll start looking into this ASAP . Would appreciate your feedback on the Jira as well

          Show
          varunthacker Varun Thacker added a comment - Hi Uwe, I created SOLR-9444 to track this. I'll start looking into this ASAP . Would appreciate your feedback on the Jira as well
          Hide
          thetaphi Uwe Schindler added a comment - - edited

          Thanks! I have seen it and already respondend. I have an proposal, but I did not look into the code fully.

          Sorry for the noise here, but URI.getPath() and URL.getPath() are 2 of the methods that are used wrongly all the time. The Maven People already have a nice blog post about that, because theis is also an issue when building classpaths and so on. If you don't read the documentation of those methods correctly, its almost always wrong. The problem is Linux, where you mostly don't see the corner cases, but people on Windows with whitespace in path names and drive letters always hit those issues. Unfortunately the "workarounds" as proposed by Stackoverflow are just invalid, because they work around the wrong use of those methods (like an X-Y-Problem), but not fixing the root cause.

          Show
          thetaphi Uwe Schindler added a comment - - edited Thanks! I have seen it and already respondend. I have an proposal, but I did not look into the code fully. Sorry for the noise here, but URI.getPath() and URL.getPath() are 2 of the methods that are used wrongly all the time. The Maven People already have a nice blog post about that, because theis is also an issue when building classpaths and so on. If you don't read the documentation of those methods correctly, its almost always wrong. The problem is Linux, where you mostly don't see the corner cases, but people on Windows with whitespace in path names and drive letters always hit those issues. Unfortunately the "workarounds" as proposed by Stackoverflow are just invalid, because they work around the wrong use of those methods (like an X-Y-Problem), but not fixing the root cause.
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Varun Thacker Now that SOLR-9444 is resolved, should we close this JIRA? Or are there any recent test failures due to this functionality?

          Show
          hgadre Hrishikesh Gadre added a comment - Varun Thacker Now that SOLR-9444 is resolved, should we close this JIRA? Or are there any recent test failures due to this functionality?
          Hide
          varunthacker Varun Thacker added a comment -

          We should resolve this issue since this feature has been released.

          Secondly I see the operation is being retried on failure. That doesn't look good.

          This was fixed in SOLR-9445

          If ant test -Dtestcase=TestHdfsCloudBackupRestore -Dtests.method=test -Dtests.seed=9470C0688BFAF297 -Dtests.slow=true -Dtests.linedocsfile=/home/jenkins/lucene-data/enwiki.random.lines.txt -Dtests.locale=es-MX -Dtests.timezone=HST -Dtests.asserts=true -Dtests.file.encoding=UTF-8 still reproduces then lets track it on a separate Jira.

          I ran it a few times and it didn't fail for me on my Mac, I didn't see any jenkins failure on this recently as well. Maybe it was fixed as part of SOLR-9444 ?

          Steve Rowe You had mentioned that this reproduced for you. Does it still reproduce ? If yes then I'll create a separate Jira and try to fix it

          Show
          varunthacker Varun Thacker added a comment - We should resolve this issue since this feature has been released. Secondly I see the operation is being retried on failure. That doesn't look good. This was fixed in SOLR-9445 If ant test -Dtestcase=TestHdfsCloudBackupRestore -Dtests.method=test -Dtests.seed=9470C0688BFAF297 -Dtests.slow=true -Dtests.linedocsfile=/home/jenkins/lucene-data/enwiki.random.lines.txt -Dtests.locale=es-MX -Dtests.timezone=HST -Dtests.asserts=true -Dtests.file.encoding=UTF-8 still reproduces then lets track it on a separate Jira. I ran it a few times and it didn't fail for me on my Mac, I didn't see any jenkins failure on this recently as well. Maybe it was fixed as part of SOLR-9444 ? Steve Rowe You had mentioned that this reproduced for you. Does it still reproduce ? If yes then I'll create a separate Jira and try to fix it
          Hide
          steve_rowe Steve Rowe added a comment -

          Steve Rowe You had mentioned that this reproduced for you. Does it still reproduce ?

          I just tried it on trunk and it no longer reproduces for me.

          Show
          steve_rowe Steve Rowe added a comment - Steve Rowe You had mentioned that this reproduced for you. Does it still reproduce ? I just tried it on trunk and it no longer reproduces for me.
          Hide
          varunthacker Varun Thacker added a comment -

          Great! Then we don't need any more work on this

          Show
          varunthacker Varun Thacker added a comment - Great! Then we don't need any more work on this

            People

            • Assignee:
              varunthacker Varun Thacker
              Reporter:
              hgadre Hrishikesh Gadre
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development