Add status.js and its tests #238

AnkshitJain · 2017-11-21T13:54:46Z

@prasadtalasila Please review the commit and suggest the necessary changes. I have included a environmentCheck.js file in /util directory that checks whether an environment variable is defined or not.
This module can be used in other js modules as well.

prasadtalasila

@AnkshitJain please see the suggested changes

prasadtalasila · 2017-11-22T03:05:18Z

util/environmentCheck.js

+};
+
+module.exports = {
+  check,


comma at the end is not required.

The airbnb config file that we extend for eslint has this convention. The rule at eslint says we should be consistent in what we intend to follow in this case.

sounds good.

prasadtalasila · 2017-11-23T17:24:08Z

load_balancer/load_balancer.js

@@ -13,13 +13,15 @@ var http = require('http');
 var bodyParser = require('body-parser');
 var fs = require('fs');
 var sys = require('sys');
-var exec = require('child_process').exec;
+var { exec } = require('child_process');


please fix the new codeclimate issues. The apriori issues can be left alone.

prasadtalasila · 2017-11-23T17:26:11Z

load_balancer/package.json

@@ -17,7 +18,8 @@
  ],
  "author": "Rajat Agarwal",
  "contributors": [
-    "Tejas Sangol", "Gaurav Narula"
+    "Tejas Sangol",


You should add your name here

prasadtalasila · 2017-11-23T17:50:20Z

load_balancer/status.js

+      resolve(undefined);
+    }
+
+    const resultJson = {


As per line-34 of logger.js, we are to use three properties in a json file for node identity. The properties are: role, hostname and port. If we use the same three properties here as well, we would be consistent.

another advantage of receiving the role of a node is that the status.js module becomes independent of its ties to execution nodes.

prasadtalasila · 2017-11-23T17:58:22Z

load_balancer/status.js

+const logger = new Logger(loggerConfig);
+
+function getComponentStatus(node) {
+  return new Promise((resolve) => {


The closure object is too long and has too many initializations. Is it possible to split this object into two smaller objects?

A coding rule known as step down rule from Robert Martin's Clean Code book will certainly help get a good structure. In step down rule, if f2() calls f1(), then code for f2() comes first in the file followed by f1().

AnkshitJain · 2017-11-24T09:53:11Z

@prasadtalasila Please have a look at the updated commit.

prasadtalasila

@AnkshitJain please see the suggested changes.

prasadtalasila · 2017-11-24T09:56:53Z

deploy/configs/load_balancer/nodes_data_conf.json

@@ -1,22 +1,27 @@
 {


For the sake of uniformity, it is better to change all the configuration files to have complete node identities. There is going to be some redundancy in configuration which can be taken care of later during code refactoring.

Do you suggest making changes to all the execution node configuration files i.e. conf.json and scores.json?

Also to the configuration files of main_server as well.

prasadtalasila · 2017-11-24T10:06:02Z

load_balancer/load_balancer.js


 var job_queue = [];
 if(process.env.mode !== "TESTING")
 {
-  server.listen(nodes_data.host_port.port);
-  console.log("Listening at "+nodes_data.host_port.port);
+  server.listen(nodes_data.load_balancer.port);


please fix the code quality issues raised here.

The check doesn't show any new errors here. Can you tell what the error is?

You may need to install the codeclimate plugin on chrome / chromium browser to have the codeclimate issues show up on the PR page.

I expect 4 errors here:

Opening curly brace does not appear on the same line as controlling statement.

Strings must use single quote.

Unexpected string concatenation.

Infix operators must be spaced.
These are previously existing quality errors, and I haven't tried to fix them. The only reason this line has a change is because the host name property was added there and the object in the configuration file nodes_data_config.json had the property name changed.

A few more issues exist. I am not able to copy paste the consolidated list.

prasadtalasila · 2017-11-24T10:07:18Z

load_balancer/load_balancer.js

+  lbStatus.status = 'up';
+  status.checkStatus((result) => {
+    //since the request is being processed, it is assumed that load balancer is up
+    const statusJson = result;


This looks fine, but there is a better library named lodash that is good at manipulating json objects. You might want to use it.

prasadtalasila · 2017-11-24T10:13:50Z

load_balancer/status.js

@@ -0,0 +1,102 @@
+/* eslint import/no-dynamic-require: 0 */


looks like this module can make use of lodash module to simplify the logic. Please see if you can use it to reduce the code.

May be its just me, but the code of status.js module does not look readable to me. Is there a way to simplify the code of status.js?

I reviewed the lodash vs es6 debate. Its definitely worth using lodash in the project. I would rather introduce lodash more strategically by identifying the features of lodash that help us, note them down for use during refactoring. Nothing much to do now.

AnkshitJain · 2017-11-24T15:39:42Z

@prasadtalasila Please have a look at the updated commit.

prasadtalasila

@AnkshitJain please check the code review comments.

prasadtalasila · 2017-11-25T03:40:23Z

deploy/configs/load_balancer/nodes_data_conf.json

      "hostname": "localhost",
      "port": "8086"
    }
  ],
  "server_info": {
+    "role": "main_server",


The database component would have role property too. Its value is to be set to database.

prasadtalasila · 2017-11-25T03:47:10Z

deploy/playbook.yml

@@ -150,6 +150,7 @@
          - "../log/load_balancer:/log"
        env:
          LOGGERCONFIG: "/etc/util/logger.json"
+          LBCONFIG: "/etc/load_balancer/nodes_data_conf.json"


It may be better to set the configuration environment variables for all the components.

The LOGGERCONFIG variable would be used by all the components and is included as an environment variable for main_server, load_balancer and execution_nodes containers. The same cannot be said for the LBCONFIG variable. It has the node details and that is not required by the other containers.

I did not ask for adding LBCONFIG environment variable to other containers. I suggested adding mainserver configuration variable to mainserver, execution node configuration variable to execution node. Done that way, there would not be any code replacements in the tests.

prasadtalasila · 2017-11-25T04:11:03Z

load_balancer/status.js

+
+const logger = new Logger(loggerConfig);
+
+const getComponentResponse = function getComponentResponse(node, resolve) {


I assume that the resolve() callback has been put in place for testability. Does the callback serve any other purpose?

prasadtalasila · 2017-11-25T04:18:52Z

load_balancer/status.js

+    this.nodes = nodes;
+    this.promises = [];
+    for (let i = 0; i < this.nodes.length; i += 1) {
+      this.promises.push(getComponentStatus(this.nodes[i]));


Putting the invocation of getComponentStatus() only in the constructor will result in staus update at the time of object creation. This is not the desirable behavior.

At best, the code can resolve status of each component only once.

prasadtalasila · 2017-11-25T05:33:49Z

load_balancer/load_balancer.js


 var job_queue = [];
 if(process.env.mode !== "TESTING")
 {
-  server.listen(nodes_data.host_port.port);
-  console.log("Listening at "+nodes_data.host_port.port);
+  server.listen(nodes_data.load_balancer.port);


A few more issues exist. I am not able to copy paste the consolidated list.

prasadtalasila · 2017-11-25T05:59:16Z

load_balancer/status.js

@@ -0,0 +1,102 @@
+/* eslint import/no-dynamic-require: 0 */


I reviewed the lodash vs es6 debate. Its definitely worth using lodash in the project. I would rather introduce lodash more strategically by identifying the features of lodash that help us, note them down for use during refactoring. Nothing much to do now.

prasadtalasila · 2017-11-25T06:00:30Z

load_balancer/status.js

+      resolve('down');
+    } else if (body.toString() === 'true') {
+      logger.debug(`Node at ${node.hostname}:${node.port} is up and running.`);
+      resolve('up');


Is the name of the callback function consistent with the nomeclature of promises?

Yes, the promises callback are resolve and reject.

AnkshitJain · 2017-11-29T16:07:06Z

@prasadtalasila The suggested structure of having the Status class before the module private methods gives an error according to our eslint conventions. Should this error be ignored or we stick with having method declarations at the top?

prasadtalasila · 2017-11-29T16:18:03Z

@AnkshitJain Then lets follow the established conventions of ES6. Let's ignore the step down rule for now.

prasadtalasila · 2017-11-29T18:21:20Z

@AnkshitJain async/await looks good. Please use the same.

AnkshitJain · 2017-11-30T08:33:57Z

@prasadtalasila Please have a look at the updated commit. Code climate gives an error related to the code complexity. Initially when the method getComponentStatus was in two separate parts, it gave 2 errors saying the code was similar. Can you suggest what can be done in this case?

prasadtalasila · 2017-11-30T09:47:40Z

@AnkshitJain Please modify the README.md file by updating the codeclimate badge to the following code.

[![Maintainability](https://api.codeclimate.com/v1/badges/e1668db2aeb3196311bc/maintainability)](https://codeclimate.com/github/AutolabJS/AutolabJS/maintainability)

Right next to codeclimate code quality badge, you can put the code coverage badge.

[![Test Coverage](https://api.codeclimate.com/v1/badges/e1668db2aeb3196311bc/test_coverage)](https://codeclimate.com/github/AutolabJS/AutolabJS/test_coverage)

Right now, this badge shows a question mark until we put in a code coverage tool. That's okay.

prasadtalasila

@AnkshitJain I am yet to complete the code review. In the mean while, please go through the following comments.

prasadtalasila · 2017-11-30T10:33:30Z

load_balancer/status.js

+
+const logger = new Logger(loggerConfig);
+
+async function getComponentStatus(node) {


How about the following code structure? (BTW, there is a compilation error in the code snippet, please only use the idea, not the actual code)

function isValidNode(node) { hostname = node.hostname; port = node.port; role = node.role; if (hostname === undefined || port === undefined || role == undefined) return false; else return true; } async function getComponentStatus(node) { if (!isValidNode(node)) throw new InvalidNodeException(); const resultJson = Object.assign({}, node); try { const body = await request.get(`https://${node.hostname}:${node.port}/connectionCheck`); if (body.toString() === 'true') { logger.debug(`Node at ${node.hostname}:${node.port} is up and running.`); resultJson.status = 'up'; } } catch (err) { logger.error(`Error connecting to ${node.hostname}:${node.port}`); logger.error(err); resultJson.status = 'down'; }

You also need to define InvalidNodeException class. I see the value in putting both isValidNode() and InvalidNodeException in a module inside util/. That way, other components can cross check their configuration values as well.

What properties wil the error class would require apart from an error message?

just the error status message is sufficient.

prasadtalasila · 2017-11-30T10:34:20Z

load_balancer/status.js

+  return resultJson;
+}
+
+async function getPromises(nodes) {


The previous name of this function sounds better. getPromises() name is too generic and unintuitive.

prasadtalasila · 2017-11-30T10:39:49Z

main_server/main_server.js

+    Path in docker containers: "/etc/main_server/APIKeys.json"
+*/
+
+check('MSCONFIG');


The check function is checking the definition of the environment variable. It can also check for the existence of the file and the read permissions. Once the existence and permissions checks pass, we can subject the configuration file to validity check by iterating the isNodeValid() function over all the entries in the configuration file.

This is just an idea. Please suggest alternatives and improvements.

@AnkshitJain
The scope of the above suggestion is very big. I have opened a new issue #250 to keep track of the suggestion. Let's wrap up this PR with the existing check() functionality.

prasadtalasila

@AnkshitJain a few more suggestions.

prasadtalasila · 2017-11-30T10:51:16Z

util/environmentCheck.js

@@ -0,0 +1,9 @@
+const check = function check(variable) {


Tests are missing for this function. You can also think of putting in the isValidNode() and InValidNodeException in this module itself.

prasadtalasila · 2017-11-30T14:50:47Z

@AnkshitJain I am done with my review. I don't have any further suggestions to make. Everything else looks good for now.

AnkshitJain · 2017-11-30T15:55:21Z

@prasadtalasila I have modified the method name getPromises and added the isValidNode method in status.js. I will add tests for environmentCheck.js along with the work concerning the custom error class. Please have a look at the updated commit.

prasadtalasila · 2017-11-30T17:55:33Z

@AnkshitJain the latest commit looks good.

prasadtalasila · 2017-12-02T16:04:30Z

@AnkshitJain please provide a tentative timeline to finish up with the pending work on this branch.

AnkshitJain · 2017-12-03T09:59:42Z

@prasadtalasila Only the tests of environmentCheck.js are remaining, which I plan to add along with the custom error class. The tests will require adding some additional util functions. If the current PR is acceptable, it can be merged and closed, otherwise I will add the custom error class after 14th and it can be left open till then.

prasadtalasila · 2017-12-20T08:14:47Z

@AnkshitJain any progress on this work? Because of a few commits on dev branch from my side, there are merge conflicts. You have to rebase your branch.

prasadtalasila

@AnkshitJain please make the changes.

prasadtalasila · 2018-01-09T12:14:04Z

load_balancer/load_balancer.js

 }

-
-
 setInterval(function () {
  connection.query('SELECT 1' ,function(err, rows, fields) {
    console.log("keep alive query");


comment the console log line.

prasadtalasila · 2018-01-09T12:16:23Z

load_balancer/load_balancer.js

 setInterval(function () {
  connection.query('SELECT 1' ,function(err, rows, fields) {
    console.log("keep alive query");
  });
-}, 10000);
+}, 100000);


The timeout value here would be half of the socket timeout set in MySQL config file.

The default value for timeout is 8 hrs. We can have out own custom timeout value with the wait_timeout variable. It was verified and here is the same.

mysql> SHOW global VARIABLES like "wait_timeout"; +---------------+-------+ | Variable_name | Value | +---------------+-------+ | wait_timeout | 28800 | +---------------+-------+ 1 row in set (0.07 sec)

We can set the timeout value to 14400 seconds here.

prasadtalasila · 2018-01-09T12:28:21Z

tests/test.sh

 # change the config file paths in all the relevant js files
-sed -i 's/\/etc\/load_balancer/\.\.\/deploy\/configs\/load_balancer/' load_balancer/load_balancer.js
-sed -i 's/\/etc\/main_server/\.\.\/deploy\/configs\/main_server/' main_server/main_server.js
+#sed -i 's/\/etc\/load_balancer/\.\.\/deploy\/configs\/load_balancer/' load_balancer/load_balancer.js


please remove the next two commented lines. There are equivalent environment config variables.

AnkshitJain · 2018-01-09T14:07:06Z

@prasadtalasila The commit has been updated. Please have a look at it.

AnkshitJain force-pushed the status branch from ad9f69d to 7cfce6d Compare November 21, 2017 14:19

prasadtalasila requested changes Nov 23, 2017

View reviewed changes

AnkshitJain force-pushed the status branch 2 times, most recently from f3551ad to 511867c Compare November 24, 2017 09:43

prasadtalasila requested changes Nov 24, 2017

View reviewed changes

AnkshitJain force-pushed the status branch from 511867c to b013c72 Compare November 24, 2017 15:30

prasadtalasila requested changes Nov 25, 2017

View reviewed changes

AnkshitJain force-pushed the status branch 2 times, most recently from 9c693d4 to d1a3c47 Compare November 30, 2017 08:24

AnkshitJain force-pushed the status branch from 25eda8f to a53b684 Compare November 30, 2017 09:56

prasadtalasila reviewed Nov 30, 2017

View reviewed changes

AnkshitJain force-pushed the status branch 2 times, most recently from da437bc to f819b4f Compare November 30, 2017 15:46

AnkshitJain force-pushed the status branch from f819b4f to 001c987 Compare December 22, 2017 07:15

prasadtalasila requested changes Jan 9, 2018

View reviewed changes

Added status.js and its tests

b3c8bae

AnkshitJain force-pushed the status branch from 001c987 to b3c8bae Compare January 9, 2018 13:13

prasadtalasila approved these changes Jan 9, 2018

View reviewed changes

prasadtalasila merged commit 7b0e7f4 into AutolabJS:dev Jan 9, 2018

prasadtalasila mentioned this pull request Jan 9, 2018

time synchronization on all autolab components #245

Closed

AnkshitJain deleted the status branch January 28, 2018 09:49


		const logger = new Logger(loggerConfig);

		const getComponentResponse = function getComponentResponse(node, resolve) {


		const logger = new Logger(loggerConfig);

		async function getComponentStatus(node) {

Add status.js and its tests #238

Add status.js and its tests #238

Conversation

AnkshitJain commented Nov 21, 2017

prasadtalasila 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnkshitJain commented Nov 24, 2017

prasadtalasila 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

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

prasadtalasila Nov 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnkshitJain commented Nov 24, 2017

prasadtalasila 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

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

AnkshitJain commented Nov 29, 2017

prasadtalasila commented Nov 29, 2017

prasadtalasila commented Nov 29, 2017

AnkshitJain commented Nov 30, 2017

prasadtalasila commented Nov 30, 2017

prasadtalasila left a comment

Choose a reason for hiding this comment

prasadtalasila Nov 30, 2017 • edited

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

prasadtalasila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prasadtalasila commented Nov 30, 2017

AnkshitJain commented Nov 30, 2017 • edited

prasadtalasila commented Nov 30, 2017

prasadtalasila commented Dec 2, 2017

AnkshitJain commented Dec 3, 2017

prasadtalasila commented Dec 20, 2017 • edited

prasadtalasila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnkshitJain Jan 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnkshitJain commented Jan 9, 2018

prasadtalasila Nov 24, 2017 •

edited

prasadtalasila Nov 30, 2017 •

edited

AnkshitJain commented Nov 30, 2017 •

edited

prasadtalasila commented Dec 20, 2017 •

edited

AnkshitJain Jan 9, 2018 •

edited