Skip to content
This repository has been archived by the owner on Mar 27, 2021. It is now read-only.

Changes to allow the use of Elasticsearch 2x. #62

Closed
wants to merge 17 commits into from

Conversation

dbrounst
Copy link

@udoprog
Copy link
Contributor

udoprog commented Jul 22, 2016

Hey,

Thanks for the contribution!
Please fix up the Travis errors and I'll give this a review at the beginning of next week.

Before merging I'll be running the integration tests as documented in the README. If you haven't, I recommend you do this before to iron out any potential issues without having to round-trip with me.

teamcity and others added 8 commits September 6, 2016 11:49
HeroicShell should return exit code 1 in case of errors
- Fixed issues for full test suite.
- Fixed NOT query filter
- Fixed "path.home" not configured issue for unit test.
…thub

# Conflicts:
#	heroic-elasticsearch-utils/src/main/java/com/spotify/heroic/elasticsearch/TransportClientSetup.java
#	heroic-elasticsearch-utils/src/main/java/com/spotify/heroic/elasticsearch/index/SingleIndexMapping.java
#	metadata/elasticsearch/src/main/java/com/spotify/heroic/metadata/elasticsearch/MetadataBackendKV.java
#	metadata/elasticsearch/src/main/java/com/spotify/heroic/metadata/elasticsearch/MetadataBackendV1.java
#	suggest/elasticsearch/src/main/java/com/spotify/heroic/suggest/elasticsearch/SuggestBackendKV.java
#	suggest/elasticsearch/src/main/java/com/spotify/heroic/suggest/elasticsearch/SuggestBackendV1.java
# Conflicts:
#	heroic-elasticsearch-utils/src/main/java/com/spotify/heroic/elasticsearch/TransportClientSetup.java
#	heroic-elasticsearch-utils/src/main/java/com/spotify/heroic/elasticsearch/index/SingleIndexMapping.java
#	metadata/elasticsearch/src/main/java/com/spotify/heroic/metadata/elasticsearch/MetadataBackendKV.java
#	metadata/elasticsearch/src/main/java/com/spotify/heroic/metadata/elasticsearch/MetadataBackendV1.java
#	suggest/elasticsearch/src/main/java/com/spotify/heroic/suggest/elasticsearch/SuggestBackendKV.java
#	suggest/elasticsearch/src/main/java/com/spotify/heroic/suggest/elasticsearch/SuggestBackendV1.java
@codecov-io
Copy link

codecov-io commented Sep 16, 2016

Current coverage is 20.15% (diff: 21.59%)

No coverage report found for master at 96b989b.

Powered by Codecov. Last update 96b989b...730f3b2

Copy link
Contributor

@udoprog udoprog left a comment

Choose a reason for hiding this comment

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

Please rebase and remove some of the merge commits. Also avoid including the HeroicShell change since its being merged separately.

@@ -49,6 +50,8 @@ public Client setup() throws Exception {
.put("node.name", InetAddress.getLocalHost().getHostName())
.put("discovery.zen.ping.multicast.enabled", false)
.putArray("discovery.zen.ping.unicast.hosts", seeds)
// Fixing path.home not configured error in unit test
.put("path.home", "/tmp/" + UUID.randomUUID())
Copy link
Contributor

@udoprog udoprog Sep 18, 2016

Choose a reason for hiding this comment

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

Make use of Files.createTempDirectory() since it does the right thing already.

@@ -89,6 +90,8 @@ public Client setup() throws Exception {
// .put("script.groovy.sandbox.enabled",
// true)
.put("discovery.zen.ping.multicast.enabled", false)
// Fixing path.home not configured error in unit test
.put("path.home", "/tmp/" + UUID.randomUUID())
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

import static org.elasticsearch.index.query.FilterBuilders.orFilter;
import static org.elasticsearch.index.query.FilterBuilders.prefixFilter;
import static org.elasticsearch.index.query.FilterBuilders.regexpFilter;
import static org.elasticsearch.index.query.FilterBuilders.termFilter;
Copy link
Contributor

@udoprog udoprog Sep 18, 2016

Choose a reason for hiding this comment

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

Prefer static imports.

import static org.elasticsearch.index.query.FilterBuilders.notFilter;
import static org.elasticsearch.index.query.FilterBuilders.orFilter;
import static org.elasticsearch.index.query.FilterBuilders.prefixFilter;
import static org.elasticsearch.index.query.FilterBuilders.termFilter;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer static imports for the QueryBuilders API. It's how most of the API is documented and seems to be the preferred method.

import static org.elasticsearch.index.query.FilterBuilders.notFilter;
import static org.elasticsearch.index.query.FilterBuilders.prefixFilter;
import static org.elasticsearch.index.query.FilterBuilders.regexpFilter;
import static org.elasticsearch.index.query.FilterBuilders.termFilter;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer static imports.

import static org.elasticsearch.index.query.FilterBuilders.notFilter;
import static org.elasticsearch.index.query.FilterBuilders.orFilter;
import static org.elasticsearch.index.query.FilterBuilders.prefixFilter;
import static org.elasticsearch.index.query.FilterBuilders.termFilter;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer static imports.

final TransportClient client = new TransportClient(settings);
final TransportClient client = TransportClient.builder()
.settings(settings)
.addPlugin(DeleteByQueryPlugin.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm on the fringe about the use of this plugin. Will the client work (apart from the delete-by-query usage) if its not installed server side?

Alternatively, implement a query-then-delete functionality on the client which seems to be the recommended approach.

@udoprog
Copy link
Contributor

udoprog commented Jun 1, 2017

Closing in favor of #267

@udoprog udoprog closed this Jun 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants