bf: ZENKO-1144 add support for Redis sorted sets #567

philipyoo · 2018-09-14T00:35:40Z

These changes are to support use of sorted sets in Redis for crr metrics. Plan is to use hourly sorted sets to track failure metrics and keep a epoch sorted list of failure keys.

Changes in this PR:

Add wrapper for Redis sorted set methods: ZADD, ZCARD, ZRANGE, ZRANGEBYSCORE, ZREM, ZSCORE
Add wrapper for Redis methods: SET, EXISTS, TTL
StatsModel.getSortedSetCurrentHour returns array of 24 normalized hourly timestamps
StatsModel.addToSortedSet adds to a sorted set and applies expiry if adding to new sorted set

bert-e · 2018-09-14T00:35:41Z

Hello philipyoo,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get
information on this process.

Status report is not available.

bert-e · 2018-09-14T00:35:44Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

2 peers

Peer approvals must include at least 1 approval from the following list:

bennettbuchanan · 2018-09-14T00:42:53Z

lib/metrics/RedisClient.js

-        return this._client
-            .multi([['incrby', key, amount], ['expire', key, expiry]])
-            .exec(cb);
+    set(key, cb) {


Don't we not need this, since we aren't using failure keys anymore?

bennettbuchanan · 2018-09-14T00:44:42Z

lib/metrics/RedisClient.js

+    /**
+     * Get range of elements in a sorted set based off score
+     * @param {string} key - name of key
+     * @param {integer} min - min score value (inclusive) (can use "-inf")


Let's update this doc to integer|string to account for '-inf' values.

bennettbuchanan · 2018-09-14T00:50:26Z

lib/metrics/StatsModel.js

+     */
+    addToSortedSet(key, score, value, cb) {
+        // milliseconds in a day
+        const msInADay = 24 * 60 * 60 * 1000;


Minor: for readability, it seems like this should be within the if block in which it is used.

bennettbuchanan · 2018-09-14T00:52:46Z

lib/metrics/StatsModel.js

+                    ['zadd', key, score, value],
+                    ['expire', key, ttl],
+                ];
+                return this._redis.batch(cmds, cb);


It would be useful to check for the batched command errors (i.e. the array of arrays), so that this method returns an error if any of those sub commands fail as well.

sounds good, on success I'm planning on returning only the zadd return value to keep both successful returns the same

bennettbuchanan · 2018-09-14T00:55:00Z

tests/functional/metrics/StatsModel.js

+        const key = 'a-test-key';
+        const score = 100;
+        const value = 'a-value';
+        // redisClient.ttl(key, (err, res))


Extra comment.

Changes in this commit: - Add wrapper for Redis sorted set methods: ZADD, ZCARD, ZRANGE, ZRANGEBYSCORE, ZREM, ZSCORE - Add wrapper for Redis methods: EXISTS

Changes in this commit: - Helper method _normalizeTimestampByHour normalizes date to nearest hour - Helper method _setDateToPreviousHour sets date back 1 hour - method getSortedSetHours returns list of 24 normalized hourly timestamps - method getSortedSetCurrentHour returns normalized hourly timestamp based on epoch passed - method addToSortedSet adds to a sorted set and applies expiry if adding to new sorted set

bert-e · 2018-09-14T19:38:59Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/8.0

The following branches will NOT be impacted:

development/6.4
development/7.4

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of Release Engineering will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of Release Engineering now.

bert-e · 2018-09-14T19:39:09Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/8.0

The following branches have NOT changed:

development/6.4
development/7.4

Please check the status of the associated issue ZENKO-1144.

Goodbye philipyoo.

rahulreddy · 2018-09-14T19:39:25Z

lib/metrics/StatsModel.js

+     * @return {undefined}
+     */
+    addToSortedSet(key, score, value, cb) {
+        this._redis.exists(key, (err, resCode) => {


I am assuming that you are only trying to expire when it's set for the first time and it's 24 hours? Your code will be cleaner if you use EXPIREAT which takes a timestamp. Also EXPIRE takes value in seconds, the ttl you calculated is in milliseconds. If you use EXPIREAT, you can set it everytime and don't have to check if the key exists.

We only want to apply the expire if its brand new. We won't know which hour sorted set we are adding to, so I'll keep it as is except for the ms -> sec issue

bennettbuchanan reviewed Sep 14, 2018

View reviewed changes

ft: ZENKO-1144 redis wrapper for sorted sets

a3973ac

Changes in this commit: - Add wrapper for Redis sorted set methods: ZADD, ZCARD, ZRANGE, ZRANGEBYSCORE, ZREM, ZSCORE - Add wrapper for Redis methods: EXISTS

philipyoo force-pushed the bugfix/ZENKO-1144-addRedisSortedSets branch 3 times, most recently from 0b91835 to 5e2bdd2 Compare September 14, 2018 14:36

philipyoo force-pushed the bugfix/ZENKO-1144-addRedisSortedSets branch from 5e2bdd2 to 0c9c462 Compare September 14, 2018 14:40

philipyoo mentioned this pull request Sep 14, 2018

bf: ZENKO-1144 remove redis scan from crr metrics #568

Merged

JianqinWang approved these changes Sep 14, 2018

View reviewed changes

bennettbuchanan approved these changes Sep 14, 2018

View reviewed changes

bert-e merged commit 0c9c462 into development/8.0 Sep 14, 2018

rahulreddy reviewed Sep 14, 2018

View reviewed changes

jonathan-gramain deleted the bugfix/ZENKO-1144-addRedisSortedSets branch September 14, 2018 23:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bf: ZENKO-1144 add support for Redis sorted sets #567

bf: ZENKO-1144 add support for Redis sorted sets #567

philipyoo commented Sep 14, 2018

bert-e commented Sep 14, 2018

bert-e commented Sep 14, 2018

bennettbuchanan Sep 14, 2018

bennettbuchanan Sep 14, 2018

bennettbuchanan Sep 14, 2018

bennettbuchanan Sep 14, 2018 •

edited

Loading

philipyoo Sep 14, 2018

bennettbuchanan Sep 14, 2018

bert-e commented Sep 14, 2018

bert-e commented Sep 14, 2018

rahulreddy Sep 14, 2018

philipyoo Sep 14, 2018 •

edited

Loading

bf: ZENKO-1144 add support for Redis sorted sets #567

bf: ZENKO-1144 add support for Redis sorted sets #567

Conversation

philipyoo commented Sep 14, 2018

bert-e commented Sep 14, 2018

Hello philipyoo,

bert-e commented Sep 14, 2018

Waiting for approval

bennettbuchanan Sep 14, 2018

Choose a reason for hiding this comment

bennettbuchanan Sep 14, 2018

Choose a reason for hiding this comment

bennettbuchanan Sep 14, 2018

Choose a reason for hiding this comment

bennettbuchanan Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

philipyoo Sep 14, 2018

Choose a reason for hiding this comment

bennettbuchanan Sep 14, 2018

Choose a reason for hiding this comment

bert-e commented Sep 14, 2018

In the queue

bert-e commented Sep 14, 2018

rahulreddy Sep 14, 2018

Choose a reason for hiding this comment

philipyoo Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

bennettbuchanan Sep 14, 2018 •

edited

Loading

philipyoo Sep 14, 2018 •

edited

Loading