Added support for Azure cosmosDB for alarms package by sandeep-paliwal · Pull Request #163 · apache/openwhisk-package-alarms

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

            .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

+            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

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


Need to support retry for DB calls.

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

+                        reject(err);});
            }
+            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

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


indentation/formatting?

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

+            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

+        };
+        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

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


is this needed?

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

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


is this needed?

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

+        };
+        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

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


is this needed?

I will tryout without this and check.

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

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


is this needed

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

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


is this needed?

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

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


formatting?

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

+            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

+                    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

+    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

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

Conversation

sandeep-paliwal commented Aug 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonpet commented Oct 19, 2018

Uh oh!

jasonpet commented Oct 19, 2018

Uh oh!

jasonpet left a comment

Choose a reason for hiding this comment

Uh oh!

jasonpet commented Oct 19, 2018

Uh oh!

sandeep-paliwal commented Oct 22, 2018

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants