NUTCH-2757 : Indexer-elastic: add authentication options#508
NUTCH-2757 : Indexer-elastic: add authentication options#508sebastian-nagel merged 4 commits intoapache:masterfrom
Conversation
| LOG.debug("No cluster name provided so using default"); | ||
| } | ||
| //Only add username and password if password is configured | ||
| if(StringUtils.isNotBlank(password)) { |
There was a problem hiding this comment.
Great, @balashashanka! Looks good and should work.
Minor point: If possible try to apply the Nutch Eclipse Code Formatting rules and also remove trailing white space. Are you using Eclipse? If not let me know or whether you need help to set it up.
There was a problem hiding this comment.
Hi @sebastian-nagel, did the formatting.
jorgelbg
left a comment
jorgelbg
left a comment
There was a problem hiding this comment.
First of all thanks for the PR @balashashanka. I left a couple of questions/comments.
conf/index-writers.xml.template
Outdated
| <param name="cluster" value=""/> | ||
| <param name="index" value="nutch"/> | ||
| <!--Configure username and password if necessary, default user is "elastic" | ||
| <param name="user" value="user"/> |
There was a problem hiding this comment.
Nit: Perhaps we should use username instead of user. For the Solr indexer configuration, we use username and also the comment mentions username.
There was a problem hiding this comment.
Nit: perhaps here we should use as well a default elastic value for the username? That way matches the DEFAULT_USER inside the Java class.
| LOG.debug("No cluster name provided so using default"); | ||
| } | ||
| // Only add username and password if password is configured | ||
| if (StringUtils.isNotBlank(password)) { |
There was a problem hiding this comment.
On the Solr indexer, we have a dedicated auth boolean key to verify if the authentication must be set (see https://github.com/apache/nutch/blob/master/src/plugin/indexer-solr/src/java/org/apache/nutch/indexwriter/solr/SolrIndexWriter.java#L97). I'm not sure I have a strong preference for either option (the boolean auth key vs checking for the empty password key) but I would err in the side of consistency since both Indexers work in a similar way.
There was a problem hiding this comment.
Hi @jorgelbg, thanks for pointing this out. I can change the code to follow the same.
There was a problem hiding this comment.
@balashashanka sure, let's make this part the same for both indexers
sebastian-nagel
left a comment
sebastian-nagel
left a comment
There was a problem hiding this comment.
Thanks, @balashashanka - code formatting is fine now. Meanwhile I've also verified that authentication works as expected (cf. NUTCH-2778 but that's clearly another issue). Please address @jorgelbg's remarks. Thanks!
jorgelbg
left a comment
jorgelbg
left a comment
There was a problem hiding this comment.
Looks good to me! Great job! I left one tiny comment in the README but it is super Nit thing 😄.
src/plugin/indexer-elastic/README.md
Outdated
| @@ -32,10 +32,12 @@ Parameter Name | Description | Default value | |||
| --|--|-- | |||
| host | Comma-separated list of hostnames to send documents to using [TransportClient](https://static.javadoc.io/org.elasticsearch/elasticsearch/5.3.0/org/elasticsearch/client/transport/TransportClient.html). Either host and port must be defined or cluster. | | |||
There was a problem hiding this comment.
I missed that the or cluster part of the sentence is now obsolete (due to the removal) but that is not a blocker from my side. If you fix it before merging it would be great.
There was a problem hiding this comment.
Hi, kind of missed this one. I removed this in the latest commit.
3 participants
Added username and password as default credentials to the HTTP Client