Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix queue details not loading - because of switch from node-redis to ioredis #459

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix queue details not loading - because of switch from node-redis to ioredis #459

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions src/server/views/helpers/queueHelpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,35 @@ function formatBytes(num) {
return (neg ? '-' : '') + numStr + ' ' + unit;
}

function splitInfo(res) {
Copy link

Choose a reason for hiding this comment

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

(pedantic) splitInfo is overly vague – consider instead something like parseRedisServerInfo

Copy link

Choose a reason for hiding this comment

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

Similarly, res is a bit arbitrary and might imply a web response, etc. but I digress.

if (typeof res !== 'string') {
return {};
}

const serverInfo = {};
const lines = res.split('\r\n');
for (let i = 0; i < lines.length; ++i) {
if (lines[i]) {
Copy link

Choose a reason for hiding this comment

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

Could eliminate an indent-level by dropping this. Empty lines are insignificant already with the if (idx > 0) conditional and we're guaranteed to have strings given the definition of String#split(...).

Feels like a premature optimisation (esp. given that the frequency of empty lines in INFO output isn't high enough to warrant a special case each iteration; a performance loss will be seen in practice by checking truthiness each time, not that it matters in this function).

const line = lines[i].trim();
if (!line.startsWith('#')) {
const idx = line.indexOf(':');
if (idx > 0) {
serverInfo[line.substring(0, idx)] = line.substring(idx + 1);
}
}
}
}

return serverInfo;
}

const Helpers = {
getStats: async function (queue) {
const client = await queue.client;
await client.info(); // update queue.client.serverInfo
const info = await client.info(); // In node-redis this will update queue.client.serverInfo

const stats = _.pickBy(client.serverInfo, (value, key) =>
// In ioredis we need to parse this information:
const stats = _.pickBy(client.serverInfo || splitInfo(info), (value, key) =>
Copy link

Choose a reason for hiding this comment

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

I'd expect client.serverInfo || splitInfo(info) to produce erratic/unreliable behaviour.

For example, given enableReadyCheck: true (the default for ioredis currently) we will have client.serverInfo defined and thus used in this instance. This will result in the same stale data issue which #218 describes.

Of course, given enableReadyCheck: false the client.serverInfo is undefined and thus we resort to splitInfo(info). Only in this case will accurate and fresh data be presented.

It'd be better to parse each time so we've fresh data regardless the configuration of enableReadyCheck.

Copy link

Choose a reason for hiding this comment

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

As of Bull v4.0.0 we do in fact have enableReadyCheck: false as a guarantee on ioredis clients. That release is still relatively new however and it'd be preferable to solve the problem for older versions of Bull as well considering it is extremely simple to do so.

_.includes(this._usefulMetrics, key)
);
stats.used_memory = formatBytes(parseInt(stats.used_memory, 10));
Expand Down