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.

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

Investigate FileSystem access in src/server.js #53

Open
pdehaan opened this issue Apr 13, 2018 · 2 comments
Open

Investigate FileSystem access in src/server.js #53

pdehaan opened this issue Apr 13, 2018 · 2 comments

Comments

@pdehaan
Copy link
Collaborator

pdehaan commented Apr 13, 2018

Randomly trolling through the code, and trying to understand the couple of fs.* calls in /src/server.js:

function respond(req, res, next) {
const dataset = req.params.dataset;
const manifestFilename = `manifests/${dataset}.json`;
if (!fs.existsSync(manifestFilename)) {
res.writeHead(404, {'Content-Type': 'text/plain'});
res.end('Not found');
next();
return;
}

If I'm reading this correctly, ever call to server.get('/:dataset', ...) will call fs.existsSync(manifestFilename).

server.get('/:dataset', respond);

If the manifest for the dataset is found, we check Redis for a cache hit. If we have a cache hit, we return the cached data and everything is 👌.
If we have a cache miss, we call sendTransposeOutput(), which will call fs.readFile():

function sendTransposeOutput(res, dataset, manifestFilename) {
fs.readFile(manifestFilename, 'utf8', (error, contents) => {
if (error) {
res.send({
error: true,
});
console.error(error);
return;
} else {
const manifest = JSON.parse(contents);
transpose(manifest, output => {
console.log(`Setting cache for key: ${dataset}`);
redisClient.setex(dataset, cacheSeconds, JSON.stringify(output));
res.send(output);
});
}
});
}

Not sure how often we expect the manifests to change, or if we can try loading all 3 manifest JSON files at server startup and keep them cached instead of calling fs.existsSync() for each dataset request. Maybe the fs.readFile() isn't as serious if we're expecting 99.999% of the calls to be Redis cache hits and we only load the file when the cache has expired (which hopefully in production we'll have a long cache life).

@openjck
Copy link
Contributor

openjck commented Apr 13, 2018

Production does have a 24-hour cache life, so most will be cache hits, but I do feel that shouldn't stop us from optimizing cache miss performance. And this is an excellent point. In production, the manifests won't change once the application starts running.

@openjck openjck added this to the M2 milestone May 26, 2018
@openjck
Copy link
Contributor

openjck commented Aug 24, 2018

This may be obsoleted by #81.

@openjck openjck removed this from the M2 milestone Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants