Support customized tornado_settings

[bkline]

It is currently not possible to override ndscheduler's default settings for tornado without completely replacing SchedulerServer.__init__(), which is undesirable for obvious reasons (losing access to bug fixes and enhancements added to the method by Nextdoor, and -- worse -- risking future incompatibilities if ND introduces changes elsewhere in ndscheduler for which modifications must be made to the constructor). If I come up with a pull request, would you consider an enhancement which adds an optional keyword argument which could be used to pass in a dictionary of custom settings to be applied to self.tornado_settings?

[sabw8217]

Yes. There's any number of ways to do this, we could just have a TORNADO_SETTINGS dict in the default_settings.py populated from the "simple" settings and let people override that completely, or merge it with the existing settings (STATIC_DIR_PATH, etc.).

[bkline]

I've got a patch which I'll test in the next few days, and then I'll submit a PR. I've mulled over the options and decided to do it as an optional argument to the constructor. I chose that approach rather than putting the dictionary in default_settings.py because (if I'm reading the code correctly) the approach used for overrides of the values in that module would replace the dictionary wholesale. That might be appropriate for database settings, but I believe for the tornado settings it is preferable to let the user override or add a single setting value, without having to maintain the others in the dictionary. That gives you the most flexibility down the road (for example, adding a new tornado setting that you don't want to be inadvertently dropped because it wasn't in the user's dictionary). Your thoughts?

[bkline]

I just realized that (again, if I'm reading the code correctly) there's another incentive to use an optional parameter for the SchedulerServer constructor instead of putting the default dictionary of tornado settings in default_settings.py. Most of the values which the constructor is currently using for the self.tornado_settings dictionary are not available until after the ndscheduler.settings dictionary has been assembled. So we'd have a chicken-and-egg problem. Currently, the user can assume that when he or she overrides DEBUG (for example), that new value will show up in the dictionary passed to tornado. That assumption would be broken if we put the TORNADO_SETTINGS in default_settings.py. It would be possible, of course, to provide the support for this enhancement by adding all of the settings supported by tornado individually in default_settings.py, but that puts you in the awkward position of having to track changes to the set of settings supported by tornado, whereas the proposed approach would let the user override any new settings introduced by tornado without the need for any changes to ndscheduler.

[sabw8217]

@bkline the ndscheduler.settings object gets instantiated at module load time, so I think any changes you make in the settings module should show up by the time you call SchedulerServer.run(). That said, you've clearly put more thought in to this feature than I have and I don't have any objections to doing it as an optional constructor arg - in fact, if I'm going to be totally honest, my personal opinion is that this repo is (or should be) a library*, libraries should not manage "settings", and therefore everything SchedulerServer depends on should be constructor arguments, and it's up to users of the library how they want to manage whatever "settings" are necessary to instantiate the server.

But, for better or worse, that's not how it's designed right now and I'm not about to refactor it to follow that design (which would require making run an instance method and be a breaking change, with all that implies, and I just don't have time at the moment). That said, I'm certainly not about to stand in the way of moving the project in that direction.

• with the exception of the "simple_server" directory, which should probably be it's own repo anyhow