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

Fix bug in cluster mode. #14

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

VarunBatraIT
Copy link

Hi

this.cache is bind to a cluster, This means, if this.cache is updated on different cluster, key may not present in other clusters and hence returns an empty response. Consider I have two clusters, A and B

A = B = OLD
A sets the new data and updates itself as A = NEW but B doesn't update since despite it is reading from file, it relying on this.cache key exist or not.

  if (Fs.existsSync(cacheFile)) {
    data = Fs.readFileSync(cacheFile);
    data = JSON.parse(data);
    console.log("LOGGED IN CACHEMAN")
    console.log(data) //Despite there is a data here.
  } else {
    return fn(null, null);
  }

  if (!this.cache[key]) {
    //returns just because this.cache is bind to a cluster.
    console.log("EMPTY CACHE OBJECT")
    return fn(null, null);
  }

Fix bug when operating in cluster mode.
Copy link
Collaborator

@theworkflow theworkflow left a comment

Choose a reason for hiding this comment

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

cacheman-file is meant to be a local temp file store. It sounds like a better solution to store data in a cluster (as you're doing) would be redis. Try to not have the get function set data when possible. It goes against having a RESTful API. Also there are cases where data may be an empty object. Check out https://github.com/anaptfox/cacheman-file/blob/master/index.js#L52

If you can, add a failing test that describes your scenario and then some code changes to have it covered.

@VarunBatraIT
Copy link
Author

Thanks for the comment and it was very helpful.
I have written a test case which will fail. https://github.com/VarunBatraIT/cacheman-file/blob/multi-instance/test/test-multi.js
This test is for multi-instances which is as good as a cluster. It fails with the current codes.
About the rest principle, I agree to that get must not write data and I am not writing any data, I am just maintaining a record of written things which is ok to my best knowledge! This is what I have to do because we are limited by the architecture of code-base of cacheman-file. The problem is, we are relying on this.data for data to exist or not. Even when we read data, data is clearly present, we are returning null just because this.data has no key.

General Architecture of Storage in Question:
Despite that data is being stored by a different instance as long as key matches, it should get the data. I can have multiple instances in two different places rather than a global variable. This is how mysql/redis works. There can be multiple instances for both which are writing and retrieving data from the same location and it works. It doesn't matter which instance stored the data, other can retrieve it. Same logic must apply to cacheman file storage. If it was a memory storage, I would have understood since that is a problem with node.

"cacheman-file is meant to be a local temp file store" as long as it works! 👍

I completely agree with you that redis is clearly the best solution but that doesn't mean that file can't be a solution :) It fails for multi-instances in same app and clusters too.

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

Successfully merging this pull request may close these issues.

2 participants