Return error on getStats if Redis client is undefined #33
Conversation
Currently when calling getStats, if the first sync call has not been made yet, an 'cannot get value of undefined' error will be thrown. This adds an error message explaining that the client is not initialised and that an initial sync is required.
aidenkeating
force-pushed
the
error-if-undefined-redis-client
branch
from
f365e57 to
8c9d0bc
Compare
|
@david-martin @wtrocki Would you mind taking a look? Edit: The changes here are mostly indentation. These are the actual lines: L151, L174-177 |
lib/sync-metrics.js
Outdated
| if (err) { | ||
| debugError('Failed to get values from redis for key %s with error : %s', metricName, err); |
There was a problem hiding this comment.
We going to miss that debug in new version. (not sure if that was intentional and we debug callback bellow.
Gong to check that.
There was a problem hiding this comment.
I'll add a debugError statement on L175 to ensure this is logged.
That debug would be if the redis client itself had an error, the current change checks if the redis client actually exists. So we'll still hit it when relevant.
|
@aidenkeating - Awesome contribution. This change may also partially resolve this issue: Can you provide verification steps to avoid misunderstandings? |
|
@wtrocki Verification can be done using this PR. feedhenry/fh-sync-server#5
|
lib/sync-metrics.js
Outdated
| async.map(metricsToFetch, function(metric, callback) { | ||
| var metricName = metric.metricName; | ||
| redisClient.lrange([statsNamespace, metricName].join(':'), 0, MAX_NUMBER, function(err, data) { | ||
| if (redisClient) { |
There was a problem hiding this comment.
this change will work, but I think the code will be cleaner if we do it this way:
var getStats = function(cb) {
if(! redisClient) {
return cb(new Error(....));
}
//the rest of code that need no changes
}
WDYT?
|
Verified! |
Bump version to 1.0.10
aidenkeating
force-pushed
the
error-if-undefined-redis-client
branch
from
a5afe89 to
69fdec6
Compare
|
Thanks for review/verification, addressed comments, merging |
|
Published as |
Currently when calling getStats, if the first sync call has not been
made yet, an error will be thrown stating it cannot get
lrangeof undefined.This is a messy error and doesn't explain what is actually going on.
This adds an error message explaining that the client is not
initialised and that an initial sync is required.