upgrading elasticsearch to 5.x. #187

zhljen · 2017-09-05T18:28:54Z

No description provided.

tgianos · 2017-09-05T19:50:36Z

build.gradle

@@ -138,6 +138,10 @@ configure(javaProjects) {
 dependency("org.glassfish.jersey.containers:jersey-container-servlet:2.19")
 dependency("org.glassfish.jersey.media:jersey-media-json-jackson:2.19")
 dependency("org.spockframework:spock-guice:1.0-groovy-2.4")
+ /**es 5.4.1 dependencies*/
+ dependency("org.elasticsearch.client:transport:5.4.1")
+ dependency("org.apache.logging.log4j:log4j-api:2.7")


Why do we need these explicitly here?

To pin the version of elasticSearch client instead of relying on the default one. I removed the dependency of org.apache.logging.log4j:log4j-api:2.7.

tgianos · 2017-09-05T19:51:07Z

metacat-main/build.gradle

 compile("com.amazonaws:aws-java-sdk-sns")
 compile("joda-time:joda-time")
+ compile("org.apache.logging.log4j:log4j-api")


Is there a reason we're using log4j like this here instead of using slf4j/logback like everywhere else?

This is required by the elasticSearch transport client.

Then why do we have to explicitly depend on it. It does not get it transitively

It may change based on the spring boot version.
want to set it to a specific version, which the cluster is.

tgianos · 2017-09-05T19:52:12Z

metacat-main/src/main/java/com/netflix/metacat/main/configs/ElasticSearchConfig.java

 @Configuration
 @ConditionalOnProperty(value = "metacat.elasticsearch.enabled", havingValue = "true")
 public class ElasticSearchConfig {

+ private static final String TCP_CONNECTION_TIMEOUT = "60s";


With this being used only once maybe you should just have it as a local variable? Additionally this looks like something that would want to be a property with a default value.

Ok. Don't think we need to make it configurable. Roll back the code to previous.

ajoymajumdar · 2017-09-05T22:32:13Z

metacat-main/src/main/java/com/netflix/metacat/main/services/search/ElasticSearchDoc.java

@@ -51,11 +53,13 @@

 private String id;
 private Object dto;
+ private final String timestamp = String.valueOf(Instant.now().toEpochMilli());


Can we set the timestamp in the constructor and have this as null by default

Why do we want to put null as default? This field is always required.

When we read the document from elasticsearch, how will you populate this Object?

I am not aware of this use-case.Needs a bit explain how to use it after reading from the es. If we need to update a certain field, we can upload the fields explicitly.

Agreed. Moved timestamp init to constructor.

ajoymajumdar · 2017-09-05T22:43:25Z

...-main/src/main/java/com/netflix/metacat/main/services/search/ElasticSearchEventHandlers.java

- final ObjectNode node = metacatJsonLocator.emptyObjectNode();
- node.set("dataMetadata", dto.getDataMetadata());
- es.updates(ElasticSearchDoc.Type.table.name(), ids, metacatRequestContext, node);
+ final ElasticSearchDoc doc = new ElasticSearchDoc(dto.getName().toString(), dto,


Is this correct?

I am not sure what the previous code is doing to be honest. In the new world, the API does not take in the node, we can talk about it and update.

Updated the logic to previous implementation.

ajoymajumdar · 2017-09-05T22:57:48Z

metacat-main/src/main/java/com/netflix/metacat/main/services/search/ElasticSearchUtilImpl.java

+ * @param client  elastic search client
+ * @param config  config
+ * @param metacatJson  json utility
+ * @param elasticSearchMetric elastic search metric
 */
 public ElasticSearchUtilImpl(
 @Nullable final Client client,


Does the new ES client have retry logic? If so, can we use that instead of us handling the retry logic.

No it doesn't have the retry logic.

ajoymajumdar · 2017-09-05T23:00:13Z

metacat-main/src/main/java/com/netflix/metacat/main/services/search/ElasticSearchUtilImpl.java

 this.elasticSearchMetric.getElasticSearchSaveFailureCounter().increment();
 log("ElasticSearchUtil.saveToIndex", type, id, null, e.getMessage(), e, true, index);
 }
 }

 private static List<String> getIds(final SearchResponse response) {
- return FluentIterable.from(response.getHits()).transform(SearchHit::getId).toList();
+ final List<String> ret = Lists.newArrayList();


Why cant we use the Iterable?

ajoymajumdar · 2017-09-05T23:00:30Z

metacat-main/src/main/java/com/netflix/metacat/main/services/search/ElasticSearchUtilImpl.java

 }

 private <T> List<T> parseResponse(final SearchResponse response, final Class<T> valueType) {
- return FluentIterable.from(response.getHits()).transform(hit -> {
+ final List<T> ret = Lists.newArrayList();


Why cant we use the Iterable?

Yes we can. But the previous FluentIterable.from xx related implemenation doesn't allow mocking/stubbing the object in the unit test.

ajoymajumdar · 2017-09-06T22:14:54Z

metacat-main/build.gradle

@@ -74,14 +75,16 @@ dependencies {
 runtime("io.springfox:springfox-swagger-ui")
 runtime("org.webjars:hal-browser")
 runtime("org.springframework:spring-aspects")
+ runtime("org.apache.logging.log4j:log4j-api")


This can be removed since log4j-core gets log4j-api

zhljen · 2017-09-08T21:46:47Z

Bring back the previous elasticSearch using embedded es server.

ajoymajumdar · 2017-09-11T17:37:17Z

metacat-main/build.gradle

 testCompile("io.airlift:testing-mysql-server")
+ testCompile("org.apache.logging.log4j:log4j-core")


Can we change this to testRuntime?

Yes, tried that. The build process will fail using testRuntime
:metacat-main:compileTestGroovy

Unable to load class org.elasticsearch.node.Node due to missing dependency org/apache/logging/log4j/Logger

ajoymajumdar · 2017-09-11T18:28:28Z

...-main/src/main/java/com/netflix/metacat/main/services/search/ElasticSearchEventHandlers.java

 if (dto.isDataExternal()) {
 final List<String> ids = es.getTableIdsByUri(metadataType, dto.getDataUri());
 ids.remove(dto.getName().toString());
 if (!ids.isEmpty()) {
 final ObjectNode node = metacatJsonLocator.emptyObjectNode();
- node.set("dataMetadata", dto.getDataMetadata());
- es.updates(ElasticSearchDoc.Type.table.name(), ids, metacatRequestContext, node);
+ node.set(ElasticSearchDoc.Field.DATA_METADATA, dto.getDataMetadata());


why are we not setting the timestamp?

ajoymajumdar · 2017-09-11T18:32:11Z

metacat-main/src/main/java/com/netflix/metacat/main/services/search/ElasticSearchUtilImpl.java

+ * @param client  elastic search client
+ * @param config  config
+ * @param metacatJson  json utility
+ * @param elasticSearchMetric elastic search metric
 */
 public ElasticSearchUtilImpl(


We need to set the timestamp on softDelete, update and create.

ajoymajumdar · 2017-09-13T15:57:06Z

metacat-main/src/main/java/com/netflix/metacat/main/services/search/ElasticSearchUtilImpl.java

@@ -371,7 +355,7 @@ private ObjectNode toJsonObject(final ElasticSearchDoc elasticSearchDoc) {
 final QueryBuilder queryBuilder = QueryBuilders.boolQuery()
 .must(QueryBuilders.termsQuery("name.qualifiedName.tree", names))
 .must(QueryBuilders.termQuery("deleted_", false))
- .must(QueryBuilders.rangeQuery("_timestamp").lte(marker.toDate()))
+ .must(QueryBuilders.rangeQuery(ElasticSearchDoc.Field.TIMESTAMP).lte(marker.toDate()))


Does this work? We are storing the timestamp in epoch millis.

I will update to milli for comparison.

Please add a test case

ajoymajumdar · 2017-09-13T16:37:03Z

metacat-main/src/test/resources/search/mapping/metacat.json

@@ -1,6 +1,7 @@
 {
 "settings": {
- "index":{"number_of_shards":10},
+ "index":{"number_of_shards":5,
+ "max_result_window": 2147483647},


Why are we setting this property high?

We discussed yesterday, it's needed as setting the size in the query.
final SearchRequestBuilder request = client.prepareSearch(esIndex)
.setTypes(type)
.setSearchType(SearchType.QUERY_THEN_FETCH)
.setQuery(queryBuilder)
.setSize(Integer.MAX_VALUE);

ajoymajumdar · 2017-09-13T16:50:23Z

metacat-main/src/test/resources/search/mapping/metacat.json

- }
- }
+ "type": "keyword",
+ "index": true


We should set this to:
{
"inputFormat": {
"type" "text",
"index" = true,
"fields": {
"keyword": {
"type": "keyword",
"ignore_above": 256
}
}
}
}

Same applies to outputformat, owner, uri and serializationLib

ajoymajumdar · 2017-09-13T16:53:08Z

metacat-main/src/test/resources/search/mapping/metacat.json

 "user_": {
- "type": "string",


Need keyword and text like inputFormat

ajoymajumdar · 2017-09-13T16:54:36Z

metacat-main/src/test/resources/search/mapping/metacat.json

 "fields": {
 "tree": {
- "type": "string",
+ "type": "text",
 "analyzer": "paths"


Do we have to say index=true for the property 'tree'.

zhljen · 2017-09-13T21:23:33Z

Updated the mapping based on comments.

zhljen force-pushed the master_es branch from a66666d to 1c113da Compare September 5, 2017 19:09

zhljen requested review from tgianos and ajoymajumdar September 5, 2017 19:38

zhljen self-assigned this Sep 5, 2017

zhljen added the enhancement label Sep 5, 2017

zhljen added this to the 1.1.0 milestone Sep 5, 2017

tgianos reviewed Sep 5, 2017

View reviewed changes

ajoymajumdar reviewed Sep 5, 2017

View reviewed changes

ajoymajumdar reviewed Sep 6, 2017

View reviewed changes

zhljen force-pushed the master_es branch from 1b31f11 to b5ce6c5 Compare September 8, 2017 21:19

ajoymajumdar reviewed Sep 11, 2017

View reviewed changes

zhljen force-pushed the master_es branch from b5ce6c5 to ea2bec3 Compare September 11, 2017 22:07

ajoymajumdar reviewed Sep 13, 2017

View reviewed changes

upgrading elasticsearch to 5.x.

55aee49

zhljen force-pushed the master_es branch from ea2bec3 to 55aee49 Compare September 13, 2017 20:56

fixing the mapping based on comments

ceef57c

ajoymajumdar approved these changes Sep 13, 2017

View reviewed changes

zhljen merged commit a625d3b into Netflix:master Sep 13, 2017

		testCompile("io.airlift:testing-mysql-server")
		testCompile("org.apache.logging.log4j:log4j-core")

upgrading elasticsearch to 5.x. #187

upgrading elasticsearch to 5.x. #187

Conversation

zhljen commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhljen commented Sep 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhljen commented Sep 13, 2017