Elastic Integration (Phase II) #339

jonwinton · 2016-11-28T19:26:08Z

This PR Includes Breaking Changes

New Features:

Amphora now connects to Elastic Search directly rather than via a service in the implementation of Clay. For reference, here is the original method and this is the updated method.
New directory called elastic which contains helpers for parsing batch operations from the database. Directory will also house the mappings internal to Amphora (coming soon)
Added a utility to Amphora that allows you to dynamically require a module but only if the module exists. Handy for when you're checking to see if something has been added to a specific Clay implementation but is optional.
Shifted around and exposed helper methods for parsing batch ops from the dataase.

On The Implementation Side:

In any Clay implementation, you can now include a folder called search at the root of your instance and implement a more robust system of parsing data for ES. See example of implementation here.

…as the overrideClient for elastic

…into internal-mappings

jonwinton · 2016-11-28T19:38:54Z

You no longer instantiate the search service in the app.js within your Clay instance, all of that is moved into amphora. This vs this.

nelsonpecora

two comments, but 👍

nelsonpecora · 2016-11-28T19:38:51Z

lib/files.js

@@ -305,9 +334,11 @@ exports.getComponentPackage = control.memoize(getComponentPackage);
 exports.isDirectory = control.memoize(isDirectory);
 exports.tryRequire = control.memoize(tryRequire);
 exports.readFilePromise = control.memoize(readFilePromise);
+exports.getFilesPromise = control.memoize(getFilesPromise);


what's the difference between this and readFilePromise?

readFilePromise returns the contents of a file, getFilesPromise is like getFiles in that it returns all the filenames within a directory. I just needed to have a version of getFiles which returned a Promise.

ohhhh makes sense

nelsonpecora · 2016-11-28T19:39:27Z

lib/services/search.js

+//  *
+//  * @return {Promise}
+//  */
+// function createInternalIndices() {


commented out...?

I can remove this from this PR, but this function will be included in the next phase of changes whenever we actually instantiate the indices that are managed by Amphora.

ah, then it's cool to leave it in there

nelsonpecora · 2016-11-28T19:41:35Z

@jonwinton yeah, but that's not a breaking change in amphora's api, is it? You could still technically init that stuff in your clay instance (though you don't need to anymore)

gloddy · 2016-11-28T19:44:06Z

@jonwinton @nelsonpecora I'd think that's a breaking change to sites but not amphora.

gloddy · 2016-11-28T19:44:36Z

@jonwinton In the feature list:
For reference, here is the original method and this is the updated method.
Both links are the same.

jonwinton · 2016-11-28T19:48:03Z

Ah, sorry about that @gloddy. I've updated the original message.

gloddy

This looks great. Just some minor comments and questions.

gloddy · 2016-11-28T19:59:51Z

lib/elastic/db-filters.js

+ */
+function filterRefs(op) {
+  _.each(_.listDeepObjects(op, refProp), function (obj) {
+    console.log(op[refProp]);


console.log helpful in prd?

gloddy · 2016-11-28T20:08:54Z

lib/elastic/helpers.js

+ * @param {string} propertyName
+ * @returns {Function}
+ */
+function removeEmptyValuesForProperty(propertyName) {


Does this get used anywhere? Popped into my head when I saw this: https://github.com/nymag/amphora/pull/339/files#diff-6612bbbf33f2f172a7fe9e466fbfff28R214

gloddy · 2016-11-28T20:17:07Z

lib/services/search.js

@@ -339,17 +313,59 @@ function batch(ops) {
 }

 /**
- * Create the ES Client or an empty object


Comment not helpful anymore?

This is fixed, but it's not showing up here...Proof: https://github.com/nymag/amphora/blob/internal-mappings/lib/services/search.js#L316

gloddy · 2016-11-28T20:17:48Z

lib/services/search.js

+  // Create client + check the connection
+  return createClient(overrideClient)
+    .then(healthCheck)
+    // .then(createInternalIndices)


Holding this here for the next iteration?

gloddy · 2016-11-28T20:18:12Z

lib/services/search.js

@@ -339,17 +313,59 @@ function batch(ops) {
 }

 /**
- * Create the ES Client or an empty object
+ * [createClient description]


gloddy · 2016-11-28T20:21:07Z

coveralls · 2016-11-28T20:26:57Z

Coverage remained the same at 100.0% when pulling f45a97f on internal-mappings into 35bc761 on master.

cruzanmo · 2016-11-28T19:56:35Z

lib/elastic/db-filters.js

+ */
+function filterRefs(op) {
+  _.each(_.listDeepObjects(op, refProp), function (obj) {
+    console.log(op[refProp]);


Does this need to be logged? If so, could use winston.

cruzanmo · 2016-11-28T19:58:01Z

lib/elastic/db-filters.js

+ * @returns {boolean}
+ */
+function isOpForPage(op) {
+  return op.key.indexOf('/pages/') > 0;


op.key always a string? Could JSDoc that. Function name could be shortened to isPageOp.

cruzanmo · 2016-11-28T19:59:57Z

lib/elastic/db-filters.js

+ * @returns {boolean}
+ */
+function isOpForInstance(op) {
+  return op.key.indexOf('/instances/') > 0;


Same as above

cruzanmo · 2016-11-28T20:00:46Z

lib/elastic/db-filters.js

+/**
+ * True is component is published ("@published")
+ * @param {object} op
+ * @returns {boolean}


@param {string} op.key?

cruzanmo · 2016-11-28T20:03:24Z

lib/elastic/db-filters.test.js

+        expect(fn(op)).to.equal(true);
+      });
+
+      it('takes an operation and returns false if its key is\'nt a page ref', function () {


isn\'t or is not -- might want to search replace for this

cruzanmo · 2016-11-28T20:09:06Z

lib/elastic/helpers.js

+function compare(expected) {
+  return function (comparison) {
+    return expected === comparison;
+  };


Arrow function could be nice here return comparison => expected === comparison;

cruzanmo · 2016-11-28T20:13:49Z

lib/elastic/helpers.js

+function resolveReferencesForPropertyOfStringType(propertyName) {
+  return function (ops) {
+    return bluebird.all(_.map(ops, function (op) {
+      var result, value = op.value[propertyName];


always will be an op.value object?

cruzanmo · 2016-11-28T20:15:52Z

lib/elastic/helpers.js

+
+  _.each(ops, function (op) {
+    if (_.isString(op.value)) {
+      op.value = JSON.parse(op.value);


op.value is always an object or JSON string?

cruzanmo · 2016-11-28T20:23:07Z

lib/services/search.js

+ * @returns {Promise}
+ */
+function validateIndices(mappings) {
+  return bluebird.all(_.map(Object.keys(mappings), function (index) {


Could use native map since Object.keys is always an array.

The lodash _.map is still faster than native map because it uses a for loop, plus it maps to the patterns that we've already established. I'd prefer to leave it until there's a good performance reason to switch them all over.

cruzanmo · 2016-11-28T20:25:49Z

lib/services/search.js

+  return createClient(overrideClient)
+    .then(healthCheck)
+    // .then(createInternalIndices)
+    .then(setupMappings)


Does overrideDir get passed through?

overrideClient? It can be, but really it's only used in testing. The comment is updated to address this.

coveralls · 2016-11-28T20:41:42Z

Coverage decreased (-0.06%) to 99.941% when pulling 564a372 on internal-mappings into 35bc761 on master.

coveralls · 2016-11-28T21:06:11Z

Coverage remained the same at 100.0% when pulling e3ec3ce on internal-mappings into 35bc761 on master.

coveralls · 2016-11-28T21:11:02Z

Coverage remained the same at 100.0% when pulling f537eb5 on internal-mappings into 35bc761 on master.

coveralls · 2016-11-28T21:14:11Z

Coverage remained the same at 100.0% when pulling 87b416f on internal-mappings into 35bc761 on master.

cruzanmo · 2016-11-28T21:14:35Z

+1

coveralls · 2016-11-28T22:00:23Z

Coverage remained the same at 100.0% when pulling 05b3e52 on internal-mappings into 35bc761 on master.

coveralls · 2016-11-28T22:09:47Z

Coverage remained the same at 100.0% when pulling c7ee98c on internal-mappings into 35bc761 on master.

jonwinton and others added 30 commits November 11, 2016 10:02

Initial creation of internal mappings

105ef96

Adding in the parseOpValues function

dc02d3c

Adding a promise function for reading files from a directory

bb97794

Adding functions for elastic

04843d9

Stashing changes

b6b85f7

updating elastic-helpers per conversation with Dalia

88efe23

Moved elastic files to an elastic specific folder

e74f15f

Cleaning up linting pre-dalia

4159fe3

Removing unused route

b2dfb64

files + setup test fixing

55d6b56

Db filters tests

c68dc16

Fixing tests for search service with new instantiation scheme

9992be4

Fixing issue with bootstrap returning a value which was interpretted …

ee4c59f

…as the overrideClient for elastic

Merge branch 'internal-mappings' of https://github.com/nymag/amphora …

488f1d7

…into internal-mappings

Validate Indices test

e3ea709

getInstance test

3c81818

stashing changes

427af65

Helpers tests

019a9f6

Ensure helpers are passing

345abcd

Merge branch 'internal-mappings' of https://github.com/nymag/amphora …

0de7391

…into internal-mappings

Adding retrieveMappingData test

ca505bc

Updating the check for elastic mappings

3adb922

Finishing up tests, tests galore

a49dbe7

Tests for converting objects to strings

1c1cc52

Updating tests for search

60c9a7c

Adding a util for checking if a module exists when doing a require

0ddd734

Removing page list (for now)

b6cf347

Removing reference to page-list

17aec20

Test for applyOpFilters

28ac39d

Remove unncessary function, last tests

131483b

nelsonpecora approved these changes Nov 28, 2016

View reviewed changes

gloddy reviewed Nov 28, 2016

View reviewed changes

Fixing comments on PR

f45a97f

cruzanmo suggested changes Nov 28, 2016

View reviewed changes

Updating filter function names

564a372

jonwinton added 2 commits November 28, 2016 15:53

Fixing naming of filter tests

8da0b89

Addressing PR comments

e3ec3ce

Updating function comment

f537eb5

Removing unused function and updating jsdoc comment

87b416f

cruzanmo approved these changes Nov 28, 2016

View reviewed changes

gloddy approved these changes Nov 28, 2016

View reviewed changes

Removing unneeded test/fixing old test

05b3e52

Minor fixes

c7ee98c

dmabuada merged commit 6cdadf8 into master Nov 28, 2016

dmabuada deleted the internal-mappings branch November 28, 2016 23:27

nelsonpecora mentioned this pull request Dec 2, 2016

Revert "Elastic Integration (Phase II)" #341

Merged

Elastic Integration (Phase II) #339

Elastic Integration (Phase II) #339

Conversation

jonwinton commented Nov 28, 2016 • edited Loading

jonwinton commented Nov 28, 2016 • edited Loading

nelsonpecora left a 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

nelsonpecora commented Nov 28, 2016

gloddy commented Nov 28, 2016

gloddy commented Nov 28, 2016

jonwinton commented Nov 28, 2016

gloddy left a 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

jonwinton Nov 28, 2016 • edited Loading

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

gloddy commented Nov 28, 2016

coveralls commented Nov 28, 2016

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

jonwinton Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

cruzanmo commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

jonwinton commented Nov 28, 2016 •

edited

Loading

jonwinton commented Nov 28, 2016 •

edited

Loading

jonwinton Nov 28, 2016 •

edited

Loading

jonwinton Nov 28, 2016 •

edited

Loading