Skip to content

Conversation

HirokiUchikawa
Copy link
Member

@HirokiUchikawa HirokiUchikawa commented Feb 27, 2019

This makes it possible to store context data to Redis.

The design note is https://github.com/node-red/node-red/wiki/Design%3A-Persistable-Context#redis-plugin.

Install prototype

  1. Run the following command
cd ~/.node-red
npm install git+https://github.com/node-red-hitachi/node-red-context-redis
  1. Add a configuration in settings.js
contextStorage: {
    redis: {
        module: require("node-red-context-redis"),
        config: {
            // see Readme.md
        }
    }
}

I would appreciate if you could give me feedback on design, codes, following concerns, etc.

Concerns

  • How to clean unnecessary context ?
    clean function detects and removes unnecessary contexts based on activeNodes argument and data in the store.
    But when there are some data stored by others in the store, the plugin cannot determine if the data is not used.

    Options:

    1. Clean the data without caring others.
      The plugin cleans the data according to its activeNodes only. But others data are also deleted.
      memory and localfilesystem do this because their store are not shared with others.

    2. clean function receives deactiveNodes list instead of activeNodes.
      The plugin can know what is not used according to it, but we have to change codes. e.g. other plugin clean.

    3. Don't clean.
      User has to clean manually if needed.
      This prototype does not clean the data.

index.js Outdated
password: config.password,
tls: config.tls,
//Do not try to reconnect, throw an error
retry_strategy: () => undefined

Choose a reason for hiding this comment

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

Any reason not to reconnect? If this is set to undefined and the connection is lost to the db, then no attempts to reconnect will happen until the plugin is re-initialized which is at node-red container startup. I would suggest removing this line of code unless there is a reason to not reconnect.

Here is more context regarding this issue,

#2

@travisghansen
Copy link

I also would like to start using this. Has anyone done anything with the reconnect issue mentioned above?

@mannharleen
Copy link

Is this usable? Is it “ratified” by the NR core team?

@knolleary
Copy link
Member

No it is no "ratified". It is an unmerged pr that has been waiting to be reviewed for a long time. Anyone using it does so at their own risk without any support from the core team.

We still really want to move forwards with this, but it has fallen down the to-do list and not had the attention it needs.

@KazuhiroItoh
Copy link
Contributor

Updated Redis plugin.

The following has been changed.

  1. Enable clean function
    Clean was disabled because of concerns about references from multiple instances.
    Currently, this usage is not expected, so I enabled the function like other plugins.

  2. Add Redis retry_strategy option
    In response to Issue # 2, I have added an option to change retry_strategy.

@KazuhiroItoh
Copy link
Contributor

@knolleary
I would appreciate it if you could check on updating the Redis plugin.
I am making modifications to match the specifications of existing plug-ins.

@knolleary
Copy link
Member

@KazuhiroItoh I'm happy to merge this as-is without doing an in depth review at this time.

I think there will be some follow-up actions to take we can handle via individual issues and PRs.

Thanks for your work (and thanks to @HirokiUchikawa for doing the original work so long ago!)

@knolleary knolleary merged commit fdba50a into node-red:master Jun 16, 2021
@mannharleen
Copy link

The README.md still says "This is a work in progress." . Is it still so?

@KazuhiroItoh
Copy link
Contributor

Thank you for your advice.
I will correct README.md as it is not appropriate.

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.

6 participants