Can use external ES mapping instead of push mapping by davidclement90 · Pull Request #233 · JanusGraph/janusgraph

davidclement90 · 2017-04-26T11:05:29Z

This adds the possibility to use external mapping in order to use custom ES mapping.
If you set USE_EXTERNAL_MAPPINGS to true, JanusGraph will check if the mapping is good instead of push it.
Whitout this adds, even if I push a mapping JanusGraph will update it during register phase.
This feature will allows to manage mapping directly in Kibana and not with ES_CREATE_EXTRAS_NS properties or to set for example a property with are declare in JanusGraph as String to IP, or use copy-to ES feature.

Issue : #222
Signed-off-by: David Clement david.clement90@laposte.net

amcp

minor comments

amcp · 2017-05-17T09:09:40Z

-            Settings.Builder settings = Settings.builder();
+            if(useExternalMappings){
+                throw new IllegalArgumentException("Could not create index: " + indexName);
+            }else{


spaces before and after else

amcp · 2017-05-17T09:09:58Z

        if (!client.indexExists(indexName)) {

-            Settings.Builder settings = Settings.builder();
+            if(useExternalMappings){


spaces after if and before the open brace

amcp · 2017-05-17T09:10:40Z

                (map==Mapping.PREFIX_TREE && AttributeUtil.isGeo(dataType)),
                "Specified illegal mapping [%s] for data type [%s]",map,dataType);

+        if(useExternalMappings){


amcp · 2017-05-17T09:10:49Z

+            } catch (IOException e) {
+               throw new PermanentBackendException(e);
+            }
+         }else{            


amcp · 2017-05-17T09:10:55Z

+            try {
+               //We check if the externalMapping have the property 'key'
+               Map mappings = client.getMapping(indexName, store);
+               if(!mappings.containsKey(key)){


amcp · 2017-05-17T09:11:20Z

+
+import java.util.Map;
+
+public class RestIndexMappings {


add classdoc

amcp · 2017-05-28T16:03:26Z

@sjudeng I think I just unblocked this PR

sjudeng · 2017-05-28T16:29:15Z

@davidclement90 Please rebase on master now that your other PR has been merged (thanks @amcp) and then I can add a review as well.

davidclement90 · 2017-05-29T10:48:24Z

Thanks for the code style updates on #233.
I try to respect the code style manually, i do not find yet a good config for my code formatter.

sjudeng

Another great contribution! Some comments below. On the minor code style side can you double check your indentation is consistently four spaces everywhere?

sjudeng · 2017-05-29T13:38:43Z

@@ -230,20 +237,25 @@ private void checkForOrCreateIndex(Configuration config) throws IOException {
        //Create index if it does not already exist


Remove this comment (it's copied inside the below if-then).

sjudeng · 2017-05-29T13:41:16Z

@@ -230,20 +237,25 @@ private void checkForOrCreateIndex(Configuration config) throws IOException {
        //Create index if it does not already exist
        if (!client.indexExists(indexName)) {


Remove extra nesting level by combining if-thens.

if (!client.indexExists(indexName) && !useExternalMappings) { //Create index if it does not already exist } else if (!client.indexExists(indexName)) { throw new IllegalArgumentException("Could not create index: " + indexName); }

sjudeng · 2017-05-29T13:44:01Z

        port = getInterface() == ElasticSearchSetup.REST_CLIENT ? 9200 : 9300;
+        httpClient = HttpClients.createDefault();
+        try {
+           host = new HttpHost(InetAddress.getByName("127.0.0.1"), 9200);


Use four spaces for indentation

sjudeng · 2017-05-29T13:44:24Z

+        Configuration indexConfig = config.restrictTo(INDEX_NAME);
+        FileInputStream fis = null;
+        try {
+          //Test create index KO mapping is not push


Use four spaces for indentation

sjudeng · 2017-05-29T13:44:49Z

+            assertTrue(res.getStatusLine().getStatusCode() >= 200);
+            assertTrue(res.getStatusLine().getStatusCode() < 300);
+            assertFalse(EntityUtils.toString(res.getEntity()).contains("error"));
+        }finally{


} finally {

sjudeng · 2017-05-29T13:45:06Z

+
+    private void executeRequest(HttpRequestBase request) throws IOException {
+        CloseableHttpResponse res = null;
+        try{


sjudeng · 2017-05-29T13:51:16Z

+    public static final ConfigOption<Boolean> USE_EXTERNAL_MAPPINGS =
+            new ConfigOption<Boolean>(ES_CREATE_NS, "use-external-mappings",
+            "When JanusGraph register an index, JanusGraph can push the mapping or use an external mapping. " +
+            "Enabling this option make JanusGraph use an external mapping.", ConfigOption.Type.MASKABLE, false);


For description how about just "Whether JanusGraph should make use of an external mapping when registering an index." Or if you prefer yours make a few corrections: s/register/registers/, s/, JanusGraph/ it/ and s/make/makes/

sjudeng · 2017-05-29T14:14:08Z

+    public Map getMapping(String indexName, String typeName) throws IOException{
+        Response response = performRequest("GET", "/" + indexName.toLowerCase() + "/_mapping/"+typeName, null);
+        try (final InputStream inputStream = response.getEntity().getContent()) {
+             Map<String,Map<String,Map<String,RestIndexMappings>>> settings = mapper.readValue(inputStream, new TypeReference<Map<String, Map<String,Map<String,RestIndexMappings>>>>() {});


Fix indentation to four spaces. Also you can update RestIndexMappings as shown in separate comment below and then you can simplify this deserialization code as follows.

Map<String,RestIndexMappings> settings = mapper.readValue(inputStream, new TypeReference<Map<String,RestIndexMappings>>() {}); return settings.get(indexName).getMappings().get(typeName).getProperties();

sjudeng · 2017-05-29T14:17:55Z

+     public void setProperties(Map<String, Object> properties) {
+         this.properties = properties;
+     }
+}


Move this (single-type) mapping object down to inner class and use outer object for the (multi-type) mappings object.

public class RestIndexMappings { private Map<String,RestIndexMapping> mappings; public Map<String, RestIndexMapping> getMappings() { return mappings; } public void setMappings(Map<String, RestIndexMapping> mappings) { this.mappings = mappings; } public static class RestIndexMapping { private Map<String,Object> properties; public Map<String, Object> getProperties() { return properties; } public void setProperties(Map<String, Object> properties) { this.properties = properties; } } }

sjudeng · 2017-05-30T01:16:28Z

+    public static final ConfigOption<Boolean> USE_EXTERNAL_MAPPINGS =
+            new ConfigOption<Boolean>(ES_CREATE_NS, "use-external-mappings",
+            "Whether JanusGraph should make use of an external mapping when registering an index." +
+            "Enabling this option make JanusGraph use an external mapping.", ConfigOption.Type.MASKABLE, false);


Remove the second sentence ("Enabling this option...") or else add a space after the preceding period and change "make" to "makes".

sjudeng · 2017-05-30T12:38:40Z

@amcp Are your requested changes in here? I'll hold off merging #289 until this is merged since I think it's close.

sjudeng · 2017-06-03T12:01:44Z

@amcp Since it looks like you're looking over PRs now (thanks!!) can you also approve and/or merge this one if your comments are addressed?

amcp · 2017-06-06T12:32:52Z


    public static final ConfigNamespace ELASTICSEARCH_NS =
-            new ConfigNamespace(INDEX_NS, "elasticsearch", "Elasticsearch index configuration");
+            new ConfigNamespace(INDEX_NS, "elasticsearch", "Elasticsearch index configuration");    


remove rogue spaces at end of line

amcp · 2017-06-06T12:34:10Z

        port = getInterface() == ElasticSearchSetup.REST_CLIENT ? 9200 : 9300;
+        httpClient = HttpClients.createDefault();
+        try {
+            host = new HttpHost(InetAddress.getByName("127.0.0.1"), 9200);


you get the port on line 88 but ignore the value and use 9200 across the board. is this ok?

Yes it's ok. Even if JanusGraph talks to Elasticsearch with the tranport API over port 9300, the mapping or the template are pushed with the REST API over port 9200.

amcp · 2017-06-06T12:35:06Z

@davidclement90 @sjudeng I caught some more things as I reviewed the latest update.

Signed-off-by: David Clement <david.clement90@laposte.net>

…seExternalMapping Can use external ES mapping instead of push mapping

janusgraph-bot added the cla: yes This PR is compliant with the CLA label Apr 26, 2017

This was referenced Apr 26, 2017

Allows to use custom analysers in ES or Solr #223

Merged

Allow to use custom analyser with ES #222

Closed

davidclement90 force-pushed the elasticsearch-useExternalMapping branch from dd4bf0c to a33d9df Compare May 10, 2017 12:03

amcp requested changes May 17, 2017

View reviewed changes

davidclement90 force-pushed the elasticsearch-useExternalMapping branch from a33d9df to 9a05a35 Compare May 18, 2017 09:18

davidclement90 force-pushed the elasticsearch-useExternalMapping branch 2 times, most recently from bdb4079 to 8aed813 Compare May 29, 2017 10:43

sjudeng requested changes May 29, 2017

View reviewed changes

davidclement90 force-pushed the elasticsearch-useExternalMapping branch 2 times, most recently from 732343c to 7833bb0 Compare May 29, 2017 16:47

sjudeng reviewed May 30, 2017

View reviewed changes

davidclement90 force-pushed the elasticsearch-useExternalMapping branch from 7833bb0 to 403bf01 Compare May 30, 2017 06:24

sjudeng approved these changes May 30, 2017

View reviewed changes

sjudeng mentioned this pull request Jun 6, 2017

Remove core Elasticsearch dependency #289

Merged

amcp requested changes Jun 6, 2017

View reviewed changes

Use external mapping ES instead of push mapping

5ffcdb6

Signed-off-by: David Clement <david.clement90@laposte.net>

davidclement90 force-pushed the elasticsearch-useExternalMapping branch from 403bf01 to 5ffcdb6 Compare June 6, 2017 18:42

amcp approved these changes Jun 7, 2017

View reviewed changes

amcp merged commit 7ff31ce into JanusGraph:master Jun 7, 2017

davidclement90 deleted the elasticsearch-useExternalMapping branch June 7, 2017 11:50

davidclement90 mentioned this pull request Oct 13, 2017

User-defined index prioritization #270

Open

bwatson-rti-org pushed a commit to bwatson-rti-org/janusgraph that referenced this pull request Mar 9, 2019

Merge pull request JanusGraph#233 from davidclement90/elasticsearch-u…

a450a58

…seExternalMapping Can use external ES mapping instead of push mapping

micpod pushed a commit to micpod/janusgraph that referenced this pull request Nov 5, 2019

Merge pull request JanusGraph#233 from davidclement90/elasticsearch-u…

dc47ef2

…seExternalMapping Can use external ES mapping instead of push mapping

		@@ -230,20 +237,25 @@ private void checkForOrCreateIndex(Configuration config) throws IOException {
		//Create index if it does not already exist

		@@ -230,20 +237,25 @@ private void checkForOrCreateIndex(Configuration config) throws IOException {
		//Create index if it does not already exist
		if (!client.indexExists(indexName)) {

Uh oh!

Conversation

davidclement90 commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amcp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcp commented May 28, 2017

Uh oh!

sjudeng commented May 28, 2017

Uh oh!

davidclement90 commented May 29, 2017

Uh oh!

sjudeng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjudeng commented May 30, 2017

Uh oh!

sjudeng commented Jun 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidclement90 Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcp commented Jun 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davidclement90 commented Apr 26, 2017 •

edited

Loading

davidclement90 Jun 6, 2017 •

edited

Loading