Wolfgang s feature/es api 5.0 v2

[wolfgang-s]

@pulkitsinghal @aquid This looks promising, I think you can merge it.

Thanks!

[pulkitsinghal]

After extensively working on the failing tests yesterday, my guess is that:

1. either we have introduced a genuine bug
2. or we have exposed a race condition where the request order is:
   1. create index, run test, delete index, next test, if index exists true then put mapping or if index does not exist then create index and then put mapping
   2. but between if index exists true and then put mapping the index gets deleted

I am not comfortable releasing a version where the test are clearly failing. And honestly I don't necessarily have the time to get this out as quick as everyone really needs 5.x so folks will just have to sit tight, contribute to fixing the test or keep using their own forked branches :(

[pulkitsinghal]

@wolfgang-s I am still working on this, finding it very difficult to give it time but haven't given up

[wolfgang-s]

I can take a look at it next week.

No worries.

[pulkitsinghal]

@aquid and @wolfgang-s

• I have added docs and folder structure to test all versions together or one-by-one.
• There is one "ghost" error which for the time being I deem as worth ignoring which happens due to race conditions in verifying/creating an index and then putting a mapping ... also another one happens mostly when Connector is initialized by testcases where a cleanup between the previous and next one hasn't taken place (would love to throw in a CURL request to destroy everything) and the self.setupMappings() screws up because its workflow is a bit different than autoMigrate
• There is one unresolved test failure worth looking into for v2.x

  TRACE: 2017-04-22T06:18:28Z
  -> POST http://localhost:9203/shakespeare/User/_update_by_query?refresh=true
  {
  "query": {
    "bool": {
      "must": [
        {
          "match": {
            "_id": 10
          }
        }
      ]
    }
  },
  "script": {
    "inline": "ctx._source.order= order;",
    "params": {
      "order": 10
    }
  }
  }
  <- 500
  {
  "error": {
    "root_cause": [
      {
        "type": "script_exception",
        "reason": "compile error",
        "script_stack": [
          "ctx._source.order= order;",
          "                   ^---- HERE"
        ],
        "script": "ctx._source.order= order;",
        "lang": "painless"
      }
    ],
    "type": "script_exception",
    "reason": "compile error",
    "script_stack": [
      "ctx._source.order= order;",
      "                   ^---- HERE"
    ],
    "script": "ctx._source.order= order;",
    "lang": "painless",
    "caused_by": {
      "type": "illegal_argument_exception",
      "reason": "Variable [order] is not defined."
    }
  },
  "status": 500
  }

• There are two unresolved test failures worth looking into for v5.x

  TRACE: 2017-04-22T06:21:22Z
  -> POST http://localhost:9203/shakespeare/PostWithId/_search?size=50
  {
  "sort": [
    "id"
  ],
  "query": {
    "bool": {
      "must": [
        {
          "match": {
            "title": "Posts"
          }
        }
      ]
    }
  }
  }
  <- 400
  {
  "error": {
    "root_cause": [
      {
        "type": "illegal_argument_exception",
        "reason": "Fielddata is disabled on text fields by default. Set fielddata=true on [id] in order to load fielddata in memory by uninverting the inverted index. Note that this can however use significant memory."
      }
    ],
    "type": "search_phase_execution_exception",
    "reason": "all shards failed",
    "phase": "query",
    "grouped": true,
    "failed_shards": [
      {
        "shard": 0,
        "index": "shakespeare",
        "node": "BKqFuny1TleiQvY4kRfLug",
        "reason": {
          "type": "illegal_argument_exception",
          "reason": "Fielddata is disabled on text fields by default. Set fielddata=true on [id] in order to load fielddata in memory by uninverting the inverted index. Note that this can however use significant memory."
        }
      }
    ],
    "caused_by": {
      "type": "illegal_argument_exception",
      "reason": "Fielddata is disabled on text fields by default. Set fielddata=true on [id] in order to load fielddata in memory by uninverting the inverted index. Note that this can however use significant memory."
    }
  },
  "status": 400
  }

and

TRACE: 2017-04-22T06:21:53Z
  -> POST http://localhost:9203/shakespeare/User/_update_by_query?refresh=true
  {
    "query": {
      "bool": {
        "must": [
          {
            "match": {
              "_id": 10
            }
          }
        ]
      }
    },
    "script": {
      "inline": "ctx._source.order= order;",
      "params": {
        "order": 10
      }
    }
  }
  <- 500
  {
    "error": {
      "root_cause": [
        {
          "type": "script_exception",
          "reason": "compile error",
          "script_stack": [
            "ctx._source.order= order;",
            "                   ^---- HERE"
          ],
          "script": "ctx._source.order= order;",
          "lang": "painless"
        }
      ],
      "type": "script_exception",
      "reason": "compile error",
      "script_stack": [
        "ctx._source.order= order;",
        "                   ^---- HERE"
      ],
      "script": "ctx._source.order= order;",
      "lang": "painless",
      "caused_by": {
        "type": "illegal_argument_exception",
        "reason": "Variable [order] is not defined."
      }
    },
    "status": 500
  }

[pulkitsinghal]

so @aquid and/or @wolfgang-s ... is either one of you upto fixing some of all of the 3 remaining errors? They may be simple mis-configurations but even fixing that would go a long way in making sure our tests are reliable.

Some clues: a) https://www.elastic.co/guide/en/elasticsearch/reference/current/fielddata.html#before-enabling-fielddata b) ES server logs

es_v5_1  | org.elasticsearch.script.ScriptException: compile error
es_v5_1  |  at org.elasticsearch.painless.PainlessScriptEngineService.convertToScriptException(PainlessScriptEngineService.java:264) ~[?:?]
es_v5_1  |  at org.elasticsearch.painless.PainlessScriptEngineService.compile(PainlessScriptEngineService.java:176) ~[?:?]
es_v5_1  |  at org.elasticsearch.script.ScriptService.compile(ScriptService.java:305) ~[elasticsearch-5.3.0.jar:5.3.0]
es_v5_1  |  at org.elasticsearch.action.bulk.byscroll.AbstractAsyncBulkByScrollAction$ScriptApplier.apply(AbstractAsyncBulkByScrollAction.java:807) ~[elasticsearch-5.3.0.jar:5.3.0]
es_v5_1  |  at org.elasticsearch.action.bulk.byscroll.AbstractAsyncBulkByScrollAction$ScriptApplier.apply(AbstractAsyncBulkByScrollAction.java:782) ~[elasticsearch-5.3.0.jar:5.3.0]
es_v5_1  |  at org.elasticsearch.action.bulk.byscroll.AbstractAsyncBulkByScrollAction.buildBulk(AbstractAsyncBulkByScrollAction.java:217) [elasticsearch-5.3.0.jar:5.3.0]
es_v5_1  |  at org.elasticsearch.action.bulk.byscroll.AbstractAsyncBulkByScrollAction.prepareBulkRequest(AbstractAsyncBulkByScrollAction.java:321) [elasticsearch-5.3.0.jar:5.3.0]
es_v5_1  |  at org.elasticsearch.action.bulk.byscroll.AbstractAsyncBulkByScrollAction$1.doRun(AbstractAsyncBulkByScrollAction.java:286) [elasticsearch-5.3.0.jar:5.3.0]
es_v5_1  |  at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:613) [elasticsearch-5.3.0.jar:5.3.0]
es_v5_1  |  at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37) [elasticsearch-5.3.0.jar:5.3.0]
es_v5_1  |  at org.elasticsearch.action.bulk.byscroll.WorkingBulkByScrollTask$RunOnce.doRun(WorkingBulkByScrollTask.java:316) [elasticsearch-5.3.0.jar:5.3.0]
es_v5_1  |  at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37) [elasticsearch-5.3.0.jar:5.3.0]
es_v5_1  |  at org.elasticsearch.action.bulk.byscroll.WorkingBulkByScrollTask$DelayedPrepareBulkRequest$1.doRun(WorkingBulkByScrollTask.java:246) [elasticsearch-5.3.0.jar:5.3.0]
es_v5_1  |  at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:613) [elasticsearch-5.3.0.jar:5.3.0]
es_v5_1  |  at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37) [elasticsearch-5.3.0.jar:5.3.0]
es_v5_1  |  at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) [?:1.8.0_121]
es_v5_1  |  at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) [?:1.8.0_121]
es_v5_1  |  at java.lang.Thread.run(Thread.java:745) [?:1.8.0_121]
es_v5_1  | Caused by: java.lang.IllegalArgumentException: Variable [order] is not defined.
es_v5_1  |  at org.elasticsearch.painless.Executable$Script.compile(ctx._source.order= order;:20) ~[?:?]
es_v5_1  |  at org.elasticsearch.painless.Locals.getVariable(Locals.java:181) ~[?:?]
es_v5_1  |  at org.elasticsearch.painless.Locals.getVariable(Locals.java:179) ~[?:?]
es_v5_1  |  at org.elasticsearch.painless.node.EVariable.analyze(EVariable.java:55) ~[?:?]
es_v5_1  |  at org.elasticsearch.painless.node.EAssignment.analyzeSimple(EAssignment.java:216) ~[?:?]
es_v5_1  |  at org.elasticsearch.painless.node.EAssignment.analyze(EAssignment.java:80) ~[?:?]
es_v5_1  |  at org.elasticsearch.painless.node.SExpression.analyze(SExpression.java:56) ~[?:?]
es_v5_1  |  at org.elasticsearch.painless.node.SSource.analyze(SSource.java:191) ~[?:?]
es_v5_1  |  at org.elasticsearch.painless.node.SSource.analyze(SSource.java:163) ~[?:?]
es_v5_1  |  at org.elasticsearch.painless.Compiler.compile(Compiler.java:104) ~[?:?]
es_v5_1  |  at org.elasticsearch.painless.PainlessScriptEngineService$2.run(PainlessScriptEngineService.java:171) ~[?:?]
es_v5_1  |  at org.elasticsearch.painless.PainlessScriptEngineService$2.run(PainlessScriptEngineService.java:168) ~[?:?]
es_v5_1  |  at java.security.AccessController.doPrivileged(Native Method) ~[?:1.8.0_121]
es_v5_1  |  at org.elasticsearch.painless.PainlessScriptEngineService.compile(PainlessScriptEngineService.java:168) ~[?:?]
es_v5_1  |  ... 16 more

[wolfgang-s]

Ok, I fixed the remaining errors. Next time I will start earlier ;)

• 1 error was cause by setting {id: true} on PostWithId (<- don't really now why this was there in the first place).

• script compilation errors for es5 are because it uses painless scripting language by default and not groovy (ES 1/2), so I changed this as well to use the variable "params".

patch file:

diff --git a/lib/esConnector.js b/lib/esConnector.js
index 30dea5a..8c39a72 100644
--- a/lib/esConnector.js
+++ b/lib/esConnector.js
@@ -948,7 +948,13 @@ ESConnector.prototype.updateAll = function updateAll(model, where, data, options
    };
    _.forEach(data, function (value, key) {
        if (key !== '_id' || key !== idName) {
-           body.script.inline += 'ctx._source.' + key + '= ' + key + ';';
+           if(self.apiVersion[0] > 2) {
+               // default language for inline scripts is painless if ES 5, so this needs the extra params.
+                body.script.inline += 'ctx._source.' + key + '=params.' + key + ';';
+           } else {
+               // groovy syntax
+                body.script.inline += 'ctx._source.' + key + '=' + key + ';';
+           }
            body.script.params[key] = value;
        }
    });
diff --git a/test/es-v5/03.basic-querying-no-timeout.test.js b/test/es-v5/03.basic-querying-no-timeout.test.js
index f6b6589..b03b362 100644
--- a/test/es-v5/03.basic-querying-no-timeout.test.js
+++ b/test/es-v5/03.basic-querying-no-timeout.test.js
@@ -71,7 +71,7 @@ describe('basic-querying-no-timeout', function () {
         });

         PostWithId = db.define('PostWithId', {
-            id: {type: String, id: true}, // TODO: why is this line different from `02.basic-querying.test.js` ?
+            id: {type: String},
             title: {type: String, length: 255},
             content: {type: String}
         });

[pulkitsinghal]

@aquid - Please go ahead take all the steps required to merge this PR and publish a new release. I have run all the tests. It is good to go.

Since now we will have refresh enabled by default in the esConnector, we should update the major version number. If you or anyone else disagrees with me then that's fine, just let me know and carry on.

Definitely should document this fact in release notes regardless:

refresh is enabled by default in the esConnector for ['create', 'save', 'destroy', 'destroyAll', 'updateAttributes', 'updateOrCreate', 'updateAll'] unless you setup your datasource to prohibit this explicitly with refreshOn:[]

There are some improvements left in tests which I've created a new issue for: https://github.com/strongloop-community/loopback-connector-elastic-search/issues/96

That shouldn't stop us from releasing so please go ahead @aquid and make it a great monday for the community!

cc @wolfgang-s @saurabh73 @jamesjara