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

Adds settings endpoint to configure loglevel #1418

Closed
wants to merge 1 commit into from

Conversation

flovilmart
Copy link
Contributor

Basic settings endpoint that configures logLevel

@drew-gross
Copy link
Contributor

Calling @nlutsenko @gfosco @lacker who all like to discuss the details of which endpoints to add.

@codecov-io
Copy link

Current coverage is 92.79%

Merging #1418 into master will not affect coverage as of bc96f0b

@@            master   #1418   diff @@
======================================
  Files           86      86       
  Stmts         5441    5441       
  Branches      1004    1004       
  Methods          0       0       
======================================
  Hit           5049    5049       
  Partial         10      10       
  Missed         382     382       

Review entire Coverage Diff as of bc96f0b

Powered by Codecov. Updated on successful CI builds.

@flovilmart
Copy link
Contributor Author

What we could easily to it to plug i on the Config.

As we know we can dynamically load/update the config at runtime from our tests, we could add the following:

GET:

  • appId, masterKey etc...
  • customPages for email
  • logLevel

POST:

  • customPages for email
  • iOS certificates
  • logLevel

@drew-gross
Copy link
Contributor

Yeah I think a lot of these could be really useful to the beginner devs who do a 1-click deploy on some parse hosting site. Then they can configure stuff via the dashboard. One point of discussion: I think the config provided in the ParseServer initialization should be the source of truth. If you try to change it via this endpoint when it's already set in the config, that shouldn't work. Also, in most cases the setting should survive a restart i.e. it has to go into the DB.

@flovilmart
Copy link
Contributor Author

Definitely has to go to a DB at some point... I was thinking to implement a config loader that would fetch a JSON file.

@mamaso
Copy link

mamaso commented Apr 8, 2016

I disagree with the ParseServer initialization config being the absolute source of truth.

For example, the setting 'allowClientClassCreation': when I write a parse server deployment template, I'm going to configure that setting to false by default. It's useful to be on for dev and testing, but there's too much danger of malicious clients to have it on as the default when my template users may not know about it.

Perhaps this is more of an issue of sensible defaults in the parse-server code config, but I think that most parse users would prefer to have the dashboard management view as their source of truth. It avoids the nasty bit of a code change and a server restart to modify a setting as well.

@drew-gross
Copy link
Contributor

@mamaso I think in your situation you could set the allowClientClassCreation to false in the database via this proposed endpoint when you create the deployment. Then there would be a setting only in the database, and not in the initialization code, so the user of your template could configure the setting via the dashboard, at runtime, or people could configure via initialization code. That way you get the nice experience for beginners, and also the benefits of having the initialization code be the source of truth. How does that sound?

@mamaso
Copy link

mamaso commented Apr 8, 2016

Another thing to consider: how should this work if the user is running more than one instance? Make a db request for every Config call? Refresh the cache periodically?

@drew-gross That's a fair point. For my scenario at least, I know that it will add quite a bit of overhead, initializing the database is non-trivial. Are there convincing benefits to code config as source of truth vs having settings persisted in the database? Thinking from an average parse user perspective, I don't see the benefit, and I know in my personal case as a provider it'd be more difficult to deploy. One (silly) option would be to add a meta-config option to choose whether the server config can be overwritten.

@drew-gross
Copy link
Contributor

Running more than one instance shouldn't an issue as long as the source code is the same for each instance. There would be potential issues during a rolling deploy but I don't think it would be anything too bad (ie. no potential to destroy data). If you have different code for different instances there is probably nothing we can do to help with that.

The biggest benefit I can see to code config as source of truth is that you can version it and stick it in your source control. There is also a perf benefit: if the config is provided in the source, we never need to query the database to find it. We've had issues with too many queries going to the _SCHEMA collection on every read and write, but if the schema was specified in the config, we could have only 1 query to the _SCHEMA collection, which is when the server is started and verifies that the _SCHEMA in the database matches the schema in the config.

Your suggestion of a meta-config to determine where the source of truth is actually isn't too bad, I think, it's certainly something we should consider. I doubt it would be too difficult to pre-populate a settings DB, though, especially once this endpoint exists.

@mamaso
Copy link

mamaso commented Apr 8, 2016

Some valid points on the source control & perf, and there's certainly going to be some confusion if a value is specified in source code and ignored.

I guess if it's not too difficult to implement, some settings for how this config endpoint is controlled would be quite beneficial. A few settings make sense off the top of my head:

  • allowServerConfigOverrides (let the database be the source of truth)
  • configFreshness (how old can the cached configuration be before a new Config() call prompts a call to the database-persisted configuration. Lets the user decide how they want to handle the tradeoff between performance / consistency across instances)

Names obviously totally debatable.

@drew-gross
Copy link
Contributor

I think this has been superseded by #1645, let me know if thats not the case.

@drew-gross drew-gross closed this May 9, 2016
@flovilmart
Copy link
Contributor Author

all good!

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.

5 participants