lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Hrishikesh Gadre (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (SOLR-9242) Collection level backup/restore should provide a param for specifying the repository implementation it should use
Date Mon, 04 Jul 2016 23:11:10 GMT

    [ https://issues.apache.org/jira/browse/SOLR-9242?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15361830#comment-15361830
] 

Hrishikesh Gadre commented on SOLR-9242:
----------------------------------------

[~varunthacker] Thanks for the review comments. The updated patch looks good mostly. Only
couple of minor comments,

bq.  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.

bq. 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.



> Collection level backup/restore should provide a param for specifying the repository
implementation it should use
> -----------------------------------------------------------------------------------------------------------------
>
>                 Key: SOLR-9242
>                 URL: https://issues.apache.org/jira/browse/SOLR-9242
>             Project: Solr
>          Issue Type: Improvement
>            Reporter: Hrishikesh Gadre
>            Assignee: Varun Thacker
>         Attachments: SOLR-9242.patch, SOLR-9242.patch, SOLR-9242.patch, SOLR-9242.patch
>
>
> 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.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org


Mime
View raw message