Api calls caching via redis - Closes #484 #577

SargeKhan · 2017-05-11T09:10:50Z

This pull request introduces 2 new fields in config.json file.

Boolean type cacheEnabled field for enabling/disabling cache.
Object type cache field for specifying the configuration of Redis cache.

Closes #484

Isabello

looking for the change to config.json to allow the flag to be toggled. Default option should be OFF or FALSE

Isabello · 2017-05-16T08:27:30Z

test/common/initModule.js

@@ -13,6 +13,8 @@ var database = require(path.join(dirname, '/helpers', 'database.js'));
 var genesisblock = require(path.join(dirname, '/genesisBlock.json'));
 var Logger = require(dirname + '/logger.js');
 var z_schema = require('../../helpers/z_schema.js');
+var cache = require('../../helpers/cache.js');


this naming is confusing, is there a better way? Cache and cache requires...

Yeah, having cache and Cache variable might be confusing. I'll update it to something like cacheHelper.

SargeKhan · 2017-05-16T08:39:24Z

@Isabello, I have those options in my config.json file, I'll push them and update the description of the pull request as well.

…o 484-api_calls_caching

Isabello · 2017-05-16T20:28:26Z

Thank you! Looks good. I will do another check over it in the morning

Isabello · 2017-05-17T07:44:54Z

helpers/httpApi.js

@@ -176,6 +176,24 @@ var middleware = {
 		} else {
 			next();
 		}
+	},
+	flushCache: function (logger, cache, req, res, next) {


Do we need req here?

Express calls this function internally with (req, res, next) parameters. So, (based on my knowledge), we need to specify req parameter here.

Isabello · 2017-05-17T07:46:31Z

config.json

@@ -21,6 +22,10 @@
            "error"
        ]
    },
+		"cache": {


Can you move this below API? Or even inside API since caching is related to API?

Yeah, this would definitely make more sense. I'll update this bit. Thanks!

Isabello

Please ensure that the config.json in test/ has the correct schema :)

Isabello · 2017-06-06T14:54:31Z

.gitignore

@@ -15,3 +15,5 @@ stacktrace*
 test/.coverage-unit
 tmp
 sftp-config.json
+*.swp


fine with me.

Isabello · 2017-06-06T14:54:40Z

.travis.yml

@@ -19,6 +20,7 @@ before_script:
  - tar -zxvf lisk-node-Linux-x86_64.tar.gz
  - cd test/lisk-js/; npm install; cd ../..
  - cp test/config.json test/genesisBlock.json .
+  - redis-server --port 6380 &


good thanks

Isabello · 2017-06-06T15:30:36Z

config.json

+		"redis": {
+			"host": "127.0.0.1",
+			"port": 6380,
+			"db": 0,


Isabello · 2017-06-06T16:02:10Z

schema/config.js

+						type: 'string'
+					}
+				}
+			},


Require these fields.

Isabello · 2017-06-06T16:02:16Z

schema/config.js

@@ -38,6 +38,9 @@ module.exports = {
 			topAccounts: {
 				type: 'boolean'
 			},
+			cacheEnabled: {
+				type: 'boolean'
+			},


Require this field

Isabello · 2017-06-06T16:03:53Z

test/unit/modules/cache.js

+			cache.setJsonForKey(key, value, function (err, status) {
+				expect(err).to.not.exist;
+				expect(status).to.equal('OK');
+				//this function doesn't accept a callback, so waiting 2 seconds before checking if the actions were completed


optional cb should be ok, waiting on timeout makes tests fragile

4miners · 2017-06-06T17:43:37Z

modules/cache.js

-	if(!self.isReady()) { return; }
-	async.map(['/api/blocks*', '/api/transactions*'], function (pattern) {
+Cache.prototype.onNewBlock = function (block, broadcast, cb) {
+	cb = cb ? cb: function () {};


We only call callback in one place so if (cb) { return cb(); } is more readable. If we want to keep current approach cb = cb || function () {}; is shorter and cleaner.

We use cb in multiple places based on certain conditions in other functions. So, using the second approach which you suggested. But good advice. Thanks 👍

4miners · 2017-06-07T12:00:57Z

modules/cache.js

@@ -16,14 +21,28 @@ function Cache (cb, scope) {
 	setImmediate(cb, null, self);
 }

+/*


/**, globally

SargeKhan added 3 commits May 10, 2017 02:24

added caching functionality basic structure

a935984

ignore swap files created by vi

c3f6d5e

refined cache logic, created express middleware for caching

5e18420

SargeKhan self-assigned this May 11, 2017

SargeKhan requested a review from karmacoma May 11, 2017 09:11

karmacoma changed the title ~~Api calls caching via redis - #484~~ Api calls caching via redis - Closes #484 May 11, 2017

4miners added the in progress label May 11, 2017

updated router.map to support middleware, added caching for getBroadhash

527daaa

karmacoma added the type: improvement label May 12, 2017

karmacoma changed the base branch from development to 0.9.0 May 15, 2017 12:50

SargeKhan added 3 commits May 15, 2017 23:04

updated cache error handling, added unit tests for cache module

739aab8

removed some comments

3843a40

Merge branch '0.9.0' into 484-api_calls_caching

2289adc

Isabello mentioned this pull request May 16, 2017

Implement bundled Redis LiskArchive/lisk-build#130

Closed

6 tasks

Isabello suggested changes May 16, 2017

View reviewed changes

SargeKhan added 6 commits May 16, 2017 13:48

added cache options in config.json

9ce548d

Merge branch '484-api_calls_caching' of github.com:SargeKhan/lisk int…

7711523

…o 484-api_calls_caching

added one more cache test

232b40e

added db flushing middleware, updated error handling

6ed4fc6

updated cache tests to skip when cacheEnable=false

9258f38

updated cache helper name

9aaaa32

Isabello previously approved these changes May 16, 2017

View reviewed changes

Isabello reviewed May 17, 2017

View reviewed changes

updated config.json, added middlware

6256cb2

SargeKhan dismissed Isabello’s stale review via 6256cb2 May 18, 2017 08:36

Isabello suggested changes May 18, 2017

View reviewed changes

added cache config to test config file

2427a94

MaciejBaj previously approved these changes Jun 6, 2017

View reviewed changes

Isabello suggested changes Jun 6, 2017

View reviewed changes

SargeKhan added 4 commits June 6, 2017 21:17

required redis related fields in schema

5bc07e8

add callbacks for all events listners in cache module

488d55b

add callbacks for all events listners in cache module

79cc021

remove timeouts, use callbacks for events

19fb17d

SargeKhan dismissed stale reviews from MaciejBaj and 4miners via 19fb17d June 6, 2017 17:02

4miners reviewed Jun 6, 2017

View reviewed changes

SargeKhan added 4 commits June 6, 2017 22:55

updated default cb definition

029ff44

removed orphan comment

98aee83

added docs for functions

70539a2

added some missing comments

7947002

SargeKhan added ready and removed in progress labels Jun 7, 2017

Isabello approved these changes Jun 7, 2017

View reviewed changes

4miners approved these changes Jun 7, 2017

View reviewed changes

4miners reviewed Jun 7, 2017

View reviewed changes

modules/cache.js Outdated

@@ -16,14 +21,28 @@ function Cache (cb, scope) {

setImmediate(cb, null, self);

}

/*

Copy link

Contributor

4miners Jun 7, 2017

Choose a reason for hiding this comment

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

/**, globally

Merge branch '0.9.0' into 484-api_calls_caching

1d5c273

MaciejBaj approved these changes Jun 8, 2017

View reviewed changes

4miners merged commit 5413099 into LiskArchive:0.9.0 Jun 8, 2017

karmacoma added ready and removed ready labels Jun 16, 2017

SargeKhan deleted the 484-api_calls_caching branch June 26, 2017 22:29

karmacoma removed type: bug type: improvement type: refactoring security type: test labels Aug 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Api calls caching via redis - Closes #484 #577

Api calls caching via redis - Closes #484 #577

SargeKhan commented May 11, 2017 •

edited

Loading

Isabello left a comment

Isabello May 16, 2017 •

edited

Loading

SargeKhan May 16, 2017

SargeKhan commented May 16, 2017

Isabello commented May 16, 2017

Isabello May 17, 2017

SargeKhan May 17, 2017

Isabello May 17, 2017

SargeKhan May 17, 2017

Isabello left a comment

Isabello Jun 6, 2017

Isabello Jun 6, 2017

Isabello Jun 6, 2017

Isabello Jun 6, 2017

Isabello Jun 6, 2017

Isabello Jun 6, 2017

4miners Jun 6, 2017

SargeKhan Jun 6, 2017

4miners Jun 7, 2017

Api calls caching via redis - Closes #484 #577

Api calls caching via redis - Closes #484 #577

Conversation

SargeKhan commented May 11, 2017 • edited Loading

Isabello left a comment

Choose a reason for hiding this comment

Isabello May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SargeKhan commented May 16, 2017

Isabello commented May 16, 2017

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

Isabello 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

SargeKhan commented May 11, 2017 •

edited

Loading

Isabello May 16, 2017 •

edited

Loading