-
Notifications
You must be signed in to change notification settings - Fork 151
Upgrade Elasticsearch dependency from 1.5.0 to 5.6.3 #177
Upgrade Elasticsearch dependency from 1.5.0 to 5.6.3 #177
Conversation
…ture/update-elastic-search-to-5.6.3
… integration test runs with docker image.
… clients to common module and reinstated bulk requests for rest client.
…factored touched code based on findings.
… (I presume) date format.
You may want to update the title of the PR to be "Upgrade ElasticSearch ..." instead of "Upgrade MetaModel ..." :-) It's a pretty big diff, so I'm gonna check it out locally and provide some feedback shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally like what I'm seeing here. It even makes me think that we should maybe just drop support entirely for the "native" module (now effectively the "transport protocol" module) entirely. But I guess that there's still a bit of value in keeping that.
|
||
public static final TimeValue TIMEOUT_SCROLL = TimeValue.timeValueSeconds(60); | ||
|
||
protected final Object elasticSearchClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't seem to be any reason to keep this in the abstract class any more, since the Object
type removes any kind of possibility of reusing it. It's used differently in the two subtypes, so I suggest they each just have a field for the client object with a type that they need.
elasticsearch/rest/pom.xml
Outdated
<ES_JAVA_OPTS>-Xms1g -Xmx1g</ES_JAVA_OPTS> | ||
<cluster.name>docker-cluster</cluster.name> | ||
<bootstrap.memory_lock>true</bootstrap.memory_lock> | ||
<xpack.security.enabled>false</xpack.security.enabled> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation issues here.
elasticsearch/rest/pom.xml
Outdated
<plugin> | ||
<groupId>io.fabric8</groupId> | ||
<artifactId>docker-maven-plugin</artifactId> | ||
<version>0.15.16</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fairly old version of this plugin that you're using here. Maybe we should (1) use the latest just because Docker things like this seems to move fast and (2) add it's version to in the root pom.
I suggest we do a release candidate after this PR has been merged. |
Biggest changes: