Added support for Azure cosmosDB for alarms package #163

sandeep-paliwal · 2018-08-30T08:46:53Z

Added support for alarms package to use cosmos DB.
All database interactions will now go through database.js which in turn will delegate to actual DB implementations (couchdb or cosmosdb) based on dbType.
There are some TODO comments and need to get better understanding on those.

All database interactions will now go through database.js which in turn will delegate to actual DB implementations (couchdb or cosmosdb) based on dbType There are some TODO comments and need to get better understanding on those.

tysonnorris · 2018-09-28T21:21:46Z

action/alarmWebAction.js

@@ -131,7 +131,8 @@ function main(params) {
        return new Promise(function (resolve, reject) {
            common.verifyTriggerAuth(triggerData, false)
            .then(() => {
-                db = new Database(params.DB_URL, params.DB_NAME);
+                db = getDatabase(params.DB_URL, params.DB_NAME, params.DB_TYPE,
+                    params.COSMOSDB_ROOT_DB, params.COSMOSDB_MASTERKEY);


instead of "all possible params" being in the sig, I think it might be simpler to pass a config object, which will vary based on db type, e.g.

let cosmosConfig = {type: "cosmosdb", url: "", rootdb:"", masterkey:""}; let couchConfig = {type: "couchdb", url: "", dbname:""}; getDatabase(config);

tysonnorris · 2018-09-28T21:28:31Z

action/lib/Database.js

+            else if(dbType === "cosmosdb") {
+                console.log("using cosmosdb");
+                var cosmosdb = require('./cosmosdb');
+                db = new cosmosdb(dbURL, cosmosdbMasterKey);


instead of duplicating functions like createTrigger, I think we should duplicate document manipulation functions like createDocument so that the db operations are the only piece that is delegated to the db impls.

…ll params. Fixed issue with DB follow function.

jasonpet · 2018-10-19T18:09:45Z

@sandeep-paliwal - have you tested this with couchdb? fails for me

"namespace": "whisk.system",
    "name": "alarmWebAction",
    "version": "0.0.2",
    "subject": "whisk.system",
    "activationId": "5d9dfa5f98cb4f629dfa5f98cb2f6232",
    "start": 1539971896476,
    "end": 1539971896495,
    "duration": 19,
    "response": {
        "status": "application error",
        "statusCode": 0,
        "success": false,
        "result": {
            "error": {
                "message": "Cannot read property 'getWorkerID' of undefined",
                "stack": "TypeError: Cannot read property 'getWorkerID' of undefined\n    at common.verifyTriggerAuth.then (/nodejsAction/dUGHWGoL/alarmWebAction.js:135:26)\n    at process._tickCallback (internal/process/next_tick.js:109:7)"

jasonpet · 2018-10-19T18:48:24Z

@sandeep-paliwal Please run all tests to verify it works once you fix this issue. The getDatabase function in alarmWebAction needs to be a promise that you wait on when you call it. You are returning the database but only after you wait on the initDB. The call to getDatabase right before you try to getWorkerID is undefined because you are not waiting on it.

jasonpet

Please see latest comments regarding errors.

jasonpet · 2018-10-19T19:44:41Z

@sandeep-paliwal I also see some alarms functionality that exists with couchdb that was not made available with cosmosDB as part of this PR:

The internal monitoring/health code that creates and deletes trigger db entries found in provider/lib/health.js
The trigger feed cleanup that occurs for the fire once alarm when the deleteAfterFire param is not set to false (see the handleFiredTriggers function in provider/lib/utils.js)

sandeep-paliwal · 2018-10-22T05:05:00Z

Thanks for the comments @jasonpet . I will update the PR and run the tests you have mentioned.

jsambit · 2018-11-20T05:24:42Z

provider/lib/cosmosdb.js

+        });
+    };
+
+    this.getTrigger = function(triggerID) {


Need to support retry for DB calls.

tysonnorris · 2019-01-16T00:56:01Z

action/lib/Database.js

            }
+            else
+                reject("No db type to initialize");


I think type must be couchdb or cosmosdb; found $config.type would be more clear?

Will make this change

tysonnorris · 2019-01-16T00:58:54Z

action/lib/cosmosdb.js

+                console.log("Found valid collection");
+                utilsDB.collectionLink = results[0]._self;
+                resolve(results);
+            }


indentation/formatting?

tysonnorris · 2019-01-16T01:03:13Z

action/lib/cosmosdb.js

+            client.createCollection(utilsDB.dbLink, collectionDefinition, function(err, collection) {
+                if(err) reject(err);
+
+                console.log("Created collection");


add an } else { to prevent processing after reject; or else do return reject(err);

tysonnorris · 2019-01-16T01:03:23Z

action/lib/cosmosdb.js

+        };
+        return new Promise((resolve, reject) => {
+            client.queryCollections(utilsDB.dbLink, querySpec).toArray((err, results) => {
+            if (err) reject(err);


add an } else { to prevent processing after reject; or else do return reject(err);

tysonnorris · 2019-01-16T01:05:03Z

action/lib/cosmosdb.js

+                        utilsDB.collectionLink = col._self;
+                        resolve(col);
+                    })
+                    .catch((err) => reject(err));


is this needed?

tysonnorris · 2019-01-16T01:05:27Z

action/lib/cosmosdb.js

+                    console.log("got database");
+                    resolve();
+                })
+                .catch((err) => { reject(err);});


is this needed?

tysonnorris · 2019-01-16T01:05:53Z

action/lib/cosmosdb.js

+        };
+        return new Promise((resolve, reject) => {
+        client.queryDatabases(querySpec).toArray((err, results) => {
+            if(err) reject(err);


add an } else { to prevent processing after reject; or else do return reject(err);

tysonnorris · 2019-01-16T01:06:14Z

action/lib/Database.js

+                        resolve();
+                    })
+                    .catch((err) => {
+                        reject(err);});


is this needed?

I will tryout without this and check.

tysonnorris · 2019-01-16T01:11:07Z

action/lib/cosmosdb.js

+                            .then(id => {
+                                resolve(id);
+                            })
+                            .catch(err => {


is this needed

tysonnorris · 2019-01-16T01:11:26Z

action/lib/cosmosdb.js

+                    resolve(replaced);
+                    });
+                })
+                .catch((err) => { reject(err);});


is this needed?

tysonnorris · 2019-01-16T01:12:03Z

action/lib/cosmosdb.js

+                                reject(err);
+                            });
+                        }, 1000);
+                        }


formatting?

tysonnorris · 2019-01-16T01:12:13Z

action/lib/cosmosdb.js

+            utilsDB.getTrigger(triggerID)
+                .then((doc) => {
+                    client.deleteDocument(doc._self, function(err) {
+                    if (err) reject(err);


add an } else { to prevent processing after reject; or else do return reject(err);

tysonnorris · 2019-01-16T01:12:22Z

action/lib/cosmosdb.js

+                    resolve();
+                    });
+                })
+                .catch((err) => { reject(err);});


is this needed?

Fixed issue with DB feed Fixed formatting

Added env var support for Feed check interval

asteed · 2019-03-09T00:17:44Z

provider/app.js

+    config = {
+        protocol: dbProtocol,
+        host: dbHost,
+        username: databaseName,


This should be username: dbUsername correct?

sandeep-paliwal added 3 commits August 30, 2018 14:14

Added support for Azure cosmosDB for alarms package

3f63de4

All database interactions will now go through database.js which in turn will delegate to actual DB implementations (couchdb or cosmosdb) based on dbType There are some TODO comments and need to get better understanding on those.

Fixed File formatting.

a0fa750

Fixed JS syntax issues reported by jshint.

c46c155

csantanapr added the wip label Sep 22, 2018

sandeep-paliwal added 2 commits September 25, 2018 10:56

Added cosmosdb impl for db.view function

dd45616

Fixed the build error

c6f7131

tysonnorris reviewed Sep 28, 2018

View reviewed changes

Made changes to get database using config object instead of passing a…

1c8bd66

…ll params. Fixed issue with DB follow function.

jasonpet requested changes Oct 19, 2018

View reviewed changes

sandeep-paliwal added 2 commits October 30, 2018 08:47

Added config support to init provider module DB

1c3c2a1

Merge branch 'master' into cosmosdb_support

ff9764a

jsambit reviewed Nov 20, 2018

View reviewed changes

provider/lib/cosmosdb.js Outdated

});

};

this.getTrigger = function(triggerID) {

Copy link

jsambit Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to support retry for DB calls.

Updated Cosmos DB implementation to support retry.

f4b7571

tysonnorris reviewed Jan 16, 2019

View reviewed changes

tysonnorris reviewed Jan 18, 2019

View reviewed changes

sandeep-paliwal added 3 commits January 24, 2019 20:32

Closing review comments from Tyson

827c481

Fixed issue with DB feed Fixed formatting

Added env var for feed check interval

604bbfd

Updated DB type error message with relevant info

804fa51

Added env var support for Feed check interval

asteed reviewed Mar 9, 2019

View reviewed changes

Fixed Config for couchDB based on comments from Andy.

b3543ef

asteed mentioned this pull request Oct 14, 2019

Changes to support CosmosDB as trigger store #201

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for Azure cosmosDB for alarms package #163

Added support for Azure cosmosDB for alarms package #163

sandeep-paliwal commented Aug 30, 2018

tysonnorris Sep 28, 2018

tysonnorris Sep 28, 2018

jasonpet commented Oct 19, 2018

jasonpet commented Oct 19, 2018

jasonpet left a comment

jasonpet commented Oct 19, 2018

sandeep-paliwal commented Oct 22, 2018

jsambit Nov 20, 2018

tysonnorris Jan 16, 2019

sandeep-paliwal Jan 21, 2019

tysonnorris Jan 16, 2019

tysonnorris Jan 16, 2019

tysonnorris Jan 16, 2019

tysonnorris Jan 16, 2019

tysonnorris Jan 16, 2019

tysonnorris Jan 16, 2019

sandeep-paliwal Jan 21, 2019

tysonnorris Jan 16, 2019

sandeep-paliwal Jan 21, 2019

tysonnorris Jan 16, 2019

tysonnorris Jan 16, 2019

tysonnorris Jan 16, 2019

tysonnorris Jan 16, 2019

tysonnorris Jan 16, 2019

asteed Mar 9, 2019

Added support for Azure cosmosDB for alarms package #163

Are you sure you want to change the base?

Added support for Azure cosmosDB for alarms package #163

Conversation

sandeep-paliwal commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonpet commented Oct 19, 2018

jasonpet commented Oct 19, 2018

jasonpet left a comment

Choose a reason for hiding this comment

jasonpet commented Oct 19, 2018

sandeep-paliwal commented Oct 22, 2018

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