From 6819b5c07b5afe75715b64c2d2f56e20c58c4c76 Mon Sep 17 00:00:00 2001 From: Vikas Agarwal Date: Mon, 20 May 2019 12:55:40 +0530 Subject: [PATCH 1/9] Testing new improved health check --- connect/connectNotificationServer.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/connect/connectNotificationServer.js b/connect/connectNotificationServer.js index 2bb28c2..7ee6e87 100644 --- a/connect/connectNotificationServer.js +++ b/connect/connectNotificationServer.js @@ -89,20 +89,20 @@ const getNotificationsForMentionedUser = (logger, eventConfig, content) => { return new Promise((resolve, reject) => { // eslint-disable-line no-unused-vars const handles = _.map(notifications, 'userHandle'); if (handles.length > 0) { - service.getUsersByHandle(handles).then((users) => { + return service.getUsersByHandle(handles).then((users) => { _.forEach(notifications, (notification) => { const mentionedUser = _.find(users, { handle: notification.userHandle }); notification.userId = mentionedUser ? mentionedUser.userId.toString() : notification.userHandle; }); resolve(notifications); - }).catch((error) => { + })/*.catch((error) => { if (logger) { logger.error(error); logger.info('Unable to send notification to mentioned user') } //resolves with empty notification which essentially means we are unable to send notification to mentioned user resolve([]); - }); + })*/; } else { resolve([]); } From 0da69cacbc709903073f80d20ab7d3e753283cd1 Mon Sep 17 00:00:00 2001 From: Vikas Agarwal Date: Mon, 20 May 2019 12:56:08 +0530 Subject: [PATCH 2/9] Deployable feature branch --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 8b2a88c..6e10c3f 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -102,7 +102,7 @@ workflows: context : org-global filters: branches: - only: [dev, 'feature/general-purpose-notifications-usage'] + only: [dev, 'feature/general-purpose-notifications-usage', 'feature/test_health_check'] - "build-prod": context : org-global filters: From 7c1bc9fb2fb80bf8ca631bb093cefe0c27d777be Mon Sep 17 00:00:00 2001 From: Vikas Agarwal Date: Mon, 20 May 2019 12:58:47 +0530 Subject: [PATCH 3/9] Testing new improved health check --- connect/service.js | 2 +- src/app.js | 29 +++++++++++++++++++++++------ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/connect/service.js b/connect/service.js index aae9f38..302fdb8 100644 --- a/connect/service.js +++ b/connect/service.js @@ -166,7 +166,7 @@ const getUsersById = (ids) => { * @return {Promise} resolves to the list of user details */ const getUsersByHandle = (handles) => { - const query = _.map(handles, (handle) => 'handle:"' + handle.trim().replace('"', '\\"') + '"').join(' OR '); + const query = _.map(handles, (handle) => 'handle:' + handle.trim()).join(' OR '); return M2m.getMachineToken(config.AUTH0_CLIENT_ID, config.AUTH0_CLIENT_SECRET) .catch((err) => { err.message = 'Error generating m2m token: ' + err.message; diff --git a/src/app.js b/src/app.js index ccdbbb3..5c7944d 100644 --- a/src/app.js +++ b/src/app.js @@ -75,20 +75,37 @@ function startKafkaConsumer(handlers, notificationServiceHandlers) { }); }); + var latestSubscriptions = null; + const check = function () { - logger.debug('Checking Health...') ; + logger.debug("Checking health"); if (!consumer.client.initialBrokers && !consumer.client.initialBrokers.length) { logger.debug('Found unhealthy Kafka Brokers...'); return false; } let connected = true; + let currentSubscriptions = consumer.subscriptions; + for(var sIdx in currentSubscriptions) { + // current subscription + let sub = currentSubscriptions[sIdx]; + // previous subscription + let prevSub = latestSubscriptions ? latestSubscriptions[sIdx] : null; + // levarage the `paused` field (https://github.com/oleksiyk/kafka/blob/master/lib/base_consumer.js#L66) to + // determine if there was a possibility of an unhandled exception. If we find paused status for the same + // topic in two consecutive health checks, we assume it was stuck because of unhandled error + if (prevSub && prevSub.paused && sub.paused) { + logger.error(`Found subscription for ${sIdx} in paused state for consecutive health checks`); + return false; + } + } + // stores the latest subscription status in global variable + latestSubscriptions = consumer.subscriptions; consumer.client.initialBrokers.forEach(conn => { - logger.debug(`url ${conn.server()} - connected=${conn.connected}`); - connected = conn.connected & connected; + logger.debug(`url ${conn.server()} - connected=${conn.connected}`) + connected = conn.connected & connected }); - logger.debug('Found all Kafka Brokers healthy...'); - return connected; - }; + return connected + } consumer .init() From 029c70023150b96c9a26deb6d0d41b16657be211 Mon Sep 17 00:00:00 2001 From: Vikas Agarwal Date: Mon, 20 May 2019 15:36:25 +0530 Subject: [PATCH 4/9] Removed using explicit constructor anti pattern which was breaking the promise chain to leave a error throw by an API method to turn in to an unhandled rejection of promise --- connect/connectNotificationServer.js | 40 +++++++++++++--------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/connect/connectNotificationServer.js b/connect/connectNotificationServer.js index 7ee6e87..f1e94e6 100644 --- a/connect/connectNotificationServer.js +++ b/connect/connectNotificationServer.js @@ -86,27 +86,25 @@ const getNotificationsForMentionedUser = (logger, eventConfig, content) => { // only one per userHandle notifications = _.uniqBy(notifications, 'userHandle'); - return new Promise((resolve, reject) => { // eslint-disable-line no-unused-vars - const handles = _.map(notifications, 'userHandle'); - if (handles.length > 0) { - return service.getUsersByHandle(handles).then((users) => { - _.forEach(notifications, (notification) => { - const mentionedUser = _.find(users, { handle: notification.userHandle }); - notification.userId = mentionedUser ? mentionedUser.userId.toString() : notification.userHandle; - }); - resolve(notifications); - })/*.catch((error) => { - if (logger) { - logger.error(error); - logger.info('Unable to send notification to mentioned user') - } - //resolves with empty notification which essentially means we are unable to send notification to mentioned user - resolve([]); - })*/; - } else { - resolve([]); - } - }); + const handles = _.map(notifications, 'userHandle'); + if (handles.length > 0) { + return service.getUsersByHandle(handles).then((users) => { + _.forEach(notifications, (notification) => { + const mentionedUser = _.find(users, { handle: notification.userHandle }); + notification.userId = mentionedUser ? mentionedUser.userId.toString() : notification.userHandle; + }); + return Promise.resolve(notifications); + }).catch((error) => { + if (logger) { + logger.error(error); + logger.info('Unable to send notification to mentioned user') + } + //resolves with empty notification which essentially means we are unable to send notification to mentioned user + return Promise.resolve([]); + }); + } else { + return Promise.resolve([]); + } }; /** From 667441e488b50773c874bf9002e5de65f82d46e1 Mon Sep 17 00:00:00 2001 From: Vikas Agarwal Date: Mon, 20 May 2019 15:37:29 +0530 Subject: [PATCH 5/9] Better to not break promise chain --- connect/connectNotificationServer.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/connect/connectNotificationServer.js b/connect/connectNotificationServer.js index f1e94e6..90286a6 100644 --- a/connect/connectNotificationServer.js +++ b/connect/connectNotificationServer.js @@ -413,10 +413,10 @@ const handler = (topic, message, logger, callback) => { } // get project details - service.getProject(projectId).then(project => { + return service.getProject(projectId).then(project => { let allNotifications = []; - Promise.all([ + return Promise.all([ // the order in this list defines the priority of notification for the SAME user // upper in this list - higher priority // NOTE: always add all handles here, they have to check by themselves: From 566fe4fde6c727e02af823caedfab90547276583 Mon Sep 17 00:00:00 2001 From: Vikas Agarwal Date: Mon, 20 May 2019 15:39:08 +0530 Subject: [PATCH 6/9] Aborting the process on any unhandled rejection to let the container know about this and restart the task --- connect/connectNotificationServer.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/connect/connectNotificationServer.js b/connect/connectNotificationServer.js index 90286a6..70b1e85 100644 --- a/connect/connectNotificationServer.js +++ b/connect/connectNotificationServer.js @@ -504,5 +504,12 @@ if (config.ENABLE_EMAILS) { // notificationServer.logger.error('Notification server errored out'); // }); + +process.on('unhandledRejection', (reason, promise) => { + console.log('Unhandled Rejection at:', promise, 'reason:', reason); + // aborts the process to let the HA of the container to restart the task + process.abort(); +}); + // if no need to init database, then directly start the server: notificationServer.startKafkaConsumers(); From 230cf6d7da3e98ba807a8d81e1cb0565f4fb816d Mon Sep 17 00:00:00 2001 From: Vikas Agarwal Date: Mon, 20 May 2019 15:40:27 +0530 Subject: [PATCH 7/9] Avoiding usage of explicit constructor anti pattern and instead use library method which takes care of most of the things with better fault tolerance and less chances of breaking the promise chain. --- connect/connectNotificationServer.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/connect/connectNotificationServer.js b/connect/connectNotificationServer.js index 70b1e85..41184bb 100644 --- a/connect/connectNotificationServer.js +++ b/connect/connectNotificationServer.js @@ -149,7 +149,7 @@ const getProjectMembersNotifications = (eventConfig, project) => { return Promise.resolve([]); } - return new Promise((resolve) => { + return Promise.promisify((callback) => { let notifications = []; const projectMembers = _.get(project, 'members', []); @@ -184,8 +184,8 @@ const getProjectMembersNotifications = (eventConfig, project) => { // only one per userId notifications = _.uniqBy(notifications, 'userId'); - resolve(notifications); - }); + callback(null, notifications); + })(); }; /** From 2c1843152c958e3c00d86d614eaf4c594325c54b Mon Sep 17 00:00:00 2001 From: Vikas Agarwal Date: Mon, 20 May 2019 16:04:15 +0530 Subject: [PATCH 8/9] Adding back the fix for escaping the reserved keywords of elastic search --- connect/service.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/connect/service.js b/connect/service.js index 302fdb8..aae9f38 100644 --- a/connect/service.js +++ b/connect/service.js @@ -166,7 +166,7 @@ const getUsersById = (ids) => { * @return {Promise} resolves to the list of user details */ const getUsersByHandle = (handles) => { - const query = _.map(handles, (handle) => 'handle:' + handle.trim()).join(' OR '); + const query = _.map(handles, (handle) => 'handle:"' + handle.trim().replace('"', '\\"') + '"').join(' OR '); return M2m.getMachineToken(config.AUTH0_CLIENT_ID, config.AUTH0_CLIENT_SECRET) .catch((err) => { err.message = 'Error generating m2m token: ' + err.message; From 7213c7d9f0be23619810dd8956b5c5011420413f Mon Sep 17 00:00:00 2001 From: Vikas Agarwal Date: Mon, 20 May 2019 16:05:23 +0530 Subject: [PATCH 9/9] Removed feature branch from deployable branches list --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 6e10c3f..8b2a88c 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -102,7 +102,7 @@ workflows: context : org-global filters: branches: - only: [dev, 'feature/general-purpose-notifications-usage', 'feature/test_health_check'] + only: [dev, 'feature/general-purpose-notifications-usage'] - "build-prod": context : org-global filters: