Added persistent settings store for dynamic settings configuration #1559
Conversation
|
|
||
| function getSettingsCollection(doc) { | ||
| return DatabaseAdapter.getDatabaseConnection(doc.settings.applicationId, doc.settings.collectionPrefix) | ||
| .adaptiveCollection('_ServerSettings'); |
There was a problem hiding this comment.
we wanna avoid direct access to the DB Adapter, so that would need to go through the DBController instead
There was a problem hiding this comment.
How best to do this? The only initialized db controller exists in the db adapter, and I don't think that I should initialize a new db controller with a new mongo storage adapter.
There was a problem hiding this comment.
Do you mean use the 'find' and other methods on the DBController as opposed to fetching the collections?
|
I'm not a huge fan of the freshness parameter, it seems like that just adds an extra knob to tweak that most people won't even want to tweak. Until we can integrate memcached or a real cache provider, I'd rather just load from the DB on every request. Also, could you add some end-to-end tests that view/change/change back some settings via HTTP request? Also that verify that the endpoint is master key only, and rejects request to modify a setting that is specified in config. |
| @@ -171,7 +177,11 @@ class ParseServer { | |||
| const userController = new UserController(emailControllerAdapter, appId, { verifyUserEmails }); | |||
| const liveQueryController = new LiveQueryController(liveQuery); | |||
|
|
|||
| if (settingsCacheOptions) { | |||
| cache.apps = PersistentSettingsStore(settingsCacheOptions, definedOptions); | |||
There was a problem hiding this comment.
What's the intended behavior upon server restart?
There was a problem hiding this comment.
If settingsCacheOptions is false there is no persistence. Otherwise, cache.apps.set() will trigger a pull from the database, update persisted settings with whatever is returned, then push to the database. However, locked & defined properties won't be updated even if they exist in the database because I attach a dummy setter to those properties.
There was a problem hiding this comment.
so how do you load a new configuration, like a new masterKey or update a property that is locked or defined?
There was a problem hiding this comment.
Defined settings are modifiable if lockDefinedSettings is false (so they will be overwritten by their persisted values).
Locked settings (like masterKey) are settings where I wasn't sure how to approach modifying them without a restart due to side effects. For example, if you change the masterKey from the parse dashboard all the sudden your experience is broken as you can't connect to the server anymore because your masterKey in the parse-dashboard-config is invalid.
The only way to update these locked settings is in the code configuration you pass to the parse server constructor.
There was a problem hiding this comment.
the other problem is the risk of error, rewriting from the passed options, and losing the changes (like masterKey) that has been changed from the dashboard.
|
After thinking about this a bit more I think that modifying the cache api to return promises will make this more flexible in whatever direction we ultimately go & is more natural to reason about & test. It will need to be the case anyway if we fetch from the db on every call. As for freshness/ttl parameter, I agree that most people won't want to tweak it, but I do think it's helpful to avoid the performance pitfalls of pulling from the db for every cache.get (which could be multiple times per request). I'll do some investigation into how invasive changing to a promises api is & try to clean up the freshness/ttl stuff (if its too complex/confusing I'll just throw it out). Also @drew-gross definitely need to add more tests, but I figured an early PR would be good to get feedback |
|
I think the best way to improve perf by avoiding DB calls would be to specify the setting in the config file. Then you don't need any DB or cache calls. Most settings should change rarely in production anyway. Also we shouldn't be able to do per-request caching so we don't need to do a DB query multiple times per request. My proposed semantics:
Then the setting change flow for your own hosted parse is to use the endpoint, or if you want to avoid calling the DB, switch to setting in config file and redeploy. If you want to switch away from using a config file (uncommon use case I think) you can modify the DB via the settings endpoint and then redeploy with the setting removed. For Parse as a service, you just don't have the config file, so you only change stuff via the settings endpoint. @mamaso I'm sure you have more context here as you are actually making Parse as a Service, so I'm curious to know what you think. |
|
I agree with 1) and 2) as you've laid them out, I have been generally following those semantics, although I'd need to change a few things to meet 2) exactly. From the PaaS perspective, I'd love to get close to the perf of the static config with a working settings endpoint, which the caching behavior helps to achieve. Primary benefit: customer never needs to touch server configuration code or redeploy, but has full control in (near) realtime. That being said, it's probably not worth the added complexity. The db-call-per-request behavior should be fine for dev/test. The performance will only be critical for high scale production customers, and in that scenario it's not too difficult to provide guidance on how to use a static server configuration. It also provides the added benefits of config versioning & preventing a production customer from breaking their server with a bad request to the settings endpoint. Also, if the switch between modifiable config & locked config can be done via an environment variable that makes for a pretty simple lock/unlock switch. |
|
If you are doing it via env var you will need to do a restart anyway though. I think we maybe don't need a locked config setting if we go with 1) and 2) as above. |
@flovilmart @drew-gross Added the cached settings logic described in #1418
PersistentSettingsStore is ugly, I'd happily take any refactoring suggestions.
Tests need some work, having problems running with a database on windows so I've mocked one out.
Passes basic ad-hoc testing.
Currently the persistence is best effort and it doesn't wait for promises to complete. Seems too invasive to change the api for cache.apps.get & cache.apps.set.