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

Dynamic configuration for lorenwest/node-config #621

Closed
wants to merge 2 commits into from

Conversation

rohitm
Copy link

@rohitm rohitm commented Oct 19, 2020

  • Reload configuration without restarting the node.js app
  • Listens for file changes and reloads the configuration

…posed to , Config constructor will watch the config folder for file changes and reload the configurion if the md5s have changed
@lorenwest
Copy link
Collaborator

It may seem like after 10 years and tens of millions of downloads, this feature should have been put in long ago. In fact, early versions of this library included this functionality, and we decided very early on that this library will not dynamically reload configurations.

The conversations are probably still in the chat archive, but the bottom line is when you introduce dynamic reloading, it significantly increases the problems, turning this into more of a database than a program config library.

Our advise is to use a real database for data that changes during the lifetime of the executable.

So much depth in this decision, and miles of conversation leading to where node-config is today.

@lorenwest lorenwest closed this Oct 19, 2020
@rohitm
Copy link
Author

rohitm commented Oct 20, 2020

Loren, Appreciate your feedback on the PR. I understand that as the creator of this library, you have a responsibility towards the community and I respect that. Pardon me for overlooking the pervious conversations about this topic. I haven't looked at them but the fact that this is still debated proves, there is need a for something like this. A full fledged ACID compliant database may not be the best place (in my opinion) to store configuration parameters that can, for instance - flip app features on the fly. Restarting node to effectuate new configuration may disrupt server connections at scale.

I will continue to work on this feature on my own fork and publish it as node-config with dynamic configuration as I'm certain that it will help a portion of your module users. I'll keep an eye out via added unit tests for the problems that you've mentioned. Cheers. - Rohit

@lorenwest
Copy link
Collaborator

My company uses node-config for bootstrapping, and a custom library for rolling out runtime features using Consul - one of many enterprise grade services for managing runtime configurations across servers. Our largest application has over 2,000 NodeJS application processes across over 100 servers in multiple data centers. It doesn't take much to see how this problem blossoms far beyond the scope of node-config.

As soon as you deliver hot reload, you'll be motivated to do it across application processes. Then across servers. Then across data centers.

My advise to the reader of this thread is whenever you begin down the path of hot reload, that's the motivation to look into existing runtime configuration solutions. There's demons down there, far beyond the scope of node-config.

@markstos
Copy link
Collaborator

I agree with @lorenwest here. For on-the-fly reloading, use a database-like product. Consul, Redis, whatever DB backend you are already using-- there are lots of options.

In our case, we sometimes do push out just config file changes to the the app... then we simply restart the Node.js app. We follow the best practice of having redundant web servers with a load balancer, so there is no user-facing downtime due to using a rolling restart.

This also keeps node-config simpler and provides certainty that app parts of the app are using the same config values.

@markstos
Copy link
Collaborator

I found lots of hits when searching issues and pull requests for the word "dynamic". Here's a discussion from 2014 about it. It was at that time that we discussed removing dynamic reloading and making the configuration immutable: #109

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.

3 participants