Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extend indexer-elastic-rest to support languages #257

Merged
merged 7 commits into from
Dec 16, 2017
Merged

Extend indexer-elastic-rest to support languages #257

merged 7 commits into from
Dec 16, 2017

Conversation

nickredmark
Copy link
Contributor

No description provided.

@lewismc lewismc self-requested a review December 13, 2017 20:17
lewismc
lewismc approved these changes Dec 13, 2017
View reviewed changes
Copy link
Member

@lewismc lewismc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
Anyone else?

@mfeltscher
Copy link
Contributor

Maybe add the new configuration option as well as a description to nutch-default.xml as well.

@lewismc
Copy link
Member

lewismc commented Dec 14, 2017 via email

@mfeltscher
Copy link
Contributor

@lewismc IMO this isn't true (and if so this wasn't the intention) - the available languages can be defined using a configuration value - see https://github.com/apache/nutch/pull/257/files#diff-408fd2f17bc9791dcbf531ffe6574a6aR106

@lewismc
Copy link
Member

lewismc commented Dec 14, 2017

Apologies @mfeltscher your absolutely right.
@nmaro can you pleaee update nutch-default.xml with the configuration entry... otherwise it will be unknown that this is a feature. Thank you

@nickredmark nickredmark force-pushed the feat/indexer-elastic-rest-languages branch from 3d12189 to 2829ad2 Compare December 15, 2017 08:43
@nickredmark nickredmark force-pushed the feat/indexer-elastic-rest-languages branch from 2829ad2 to 74d3038 Compare December 15, 2017 08:47
@nickredmark
Copy link
Contributor Author

Thanks @lewismc and @mfeltscher for the feedback, I added the default and rebased the code.

@lewismc
Copy link
Member

lewismc commented Dec 15, 2017

OK I'm +1 for this. Anyone else?

mfeltscher
mfeltscher reviewed Dec 15, 2017
View reviewed changes
...r-elastic-rest/src/java/org/apache/nutch/indexwriter/elasticrest/ElasticRestIndexWriter.java Outdated
}
}
if (exists) {
index = defaultIndex + "_" + language;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract logic to determine index name into its own function since you could also reuse it in the delete action.

@mfeltscher
Copy link
Contributor

LGTM :)

@thilohaas
Copy link

👍

@sebastian-nagel
Copy link
Contributor

Is there already a Jira issue for this pull-request? We need it for the release notes. Thanks!

@lewismc
Copy link
Member

lewismc commented Dec 16, 2017

Here we go https://issues.apache.org/jira/browse/NUTCH-2484

@lewismc lewismc merged commit 45ce310 into apache:master Dec 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfeltscher mfeltscher mfeltscher left review comments

@lewismc lewismc lewismc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nickredmark @mfeltscher @lewismc @thilohaas @sebastian-nagel