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

Expose loaded config files, make load idempotent #522

Merged
merged 3 commits into from
Jun 13, 2019

Conversation

kevin-bates
Copy link
Contributor

@kevin-bates kevin-bates commented May 8, 2019

This change enables applications to implement periodic reloads of
updated configuration files without requiring restarts. This is
intended for long-running, service-oriented, applications that
cannot afford to be shutdown yet may need to have their configurations
updated.

This change exposes the _loaded_config_files list on the application
class instance via a property. This was done so as to not allow
side-affects to creep in from clients.

More importantly, is the prevention of duplicate config file entries
that can occur when load_config_file is called multiple times. This
enables the method's idempotency - otherwise the list of loaded config
files increased on each (periodic) call.

This PR stems from the discussion of (now closed) PR #521.

This change enables applications to implement periodic reloads of
updated configuration files without requiring restarts.  This is
intended for long-running, service-oriented, applications that
cannot afford to be shutdown yet may need to have their configurations
updated.

This change exposes the _loaded_config_files list on the application
class instance via a property.  This was done so as to not allow
side-affects to creep in from clients.

More importantly, is the prevention of duplicate config file entries
that can occur when load_config_file is called multiple times. This i
enables the method's idempotency - otherwise the list of loaded config
files increased on each (periodic) call.
@rmorshea rmorshea requested a review from minrk May 8, 2019 23:28
@rmorshea
Copy link
Contributor

rmorshea commented May 8, 2019

@minrk I haven't looked at the config system in quite a while. I'd like to have you give this a once over.

@rmorshea rmorshea requested review from Carreau and removed request for minrk May 13, 2019 17:57
@rmorshea
Copy link
Contributor

@Carreau can you review this? It seems like a useful feature, however I haven't looked at the config system in a long time.

@kevin-bates
Copy link
Contributor Author

@rmorshea @minrk @Carreau - I just realized that master is for 5.0 and this change is more warranted for 4.x. Should I apply this change to the 4.x branch instead?

If it changes things, the same was meant for the (now defunct) #521. Seems like Jupyter Hub could use this and I later noticed it doesn't derive from JupyterApp, but Application directly.

Please advise.

@rmorshea
Copy link
Contributor

@takluyver help on this?

@kevin-bates
Copy link
Contributor Author

In looking into adding this to 4.3.3, I see that one of the things it depends on is also currently not in the 4.x branch. It will require that #341 also be included.

Since there hasn't been a minor release in over 2 years, it would be great if this could happen soon - it's long overdue.

The capability to periodically reload config files for long-running server applications strikes me as a no-brainer.

@rmorshea - thank you for your help on this.

I'm adding @rgbkrk to this since he was excited about #341 as well.

@rgbkrk
Copy link
Member

rgbkrk commented Jun 3, 2019

Restarting the failed jobs to see if it's just flaky (I didn't look at the results... 😬 )

@kevin-bates
Copy link
Contributor Author

Hi @Carreau - could you please review these changes? I'd like to push for a 4.3.3 release so Enterprise Gateway could implement dynamic configurations in our pending 2.0 release. Thank you.

@rmorshea rmorshea removed the request for review from Carreau June 13, 2019 06:24
@rmorshea
Copy link
Contributor

rmorshea commented Jun 13, 2019

@kevin-bates I'm gonna move this forward since it's a relatively minor change that clearly alleviates an immediate issue.

@rmorshea rmorshea merged commit c95f848 into ipython:master Jun 13, 2019
@kevin-bates
Copy link
Contributor Author

Thank you Ryan. Now the next question is how to get a 4.3.3 release produced that minimally includes this and #341? 🙂

@rmorshea
Copy link
Contributor

@kevin-bates I'll be meeting up with a couple core devs next week so we might be able to make a release then. Might even be a 5.0 release.

@rgbkrk
Copy link
Member

rgbkrk commented Jun 14, 2019

Thanks y'all

@kevin-bates
Copy link
Contributor Author

@rmorshea @rgbkrk - any ETA on a next release - whether that be 4.3.x or 5.0? If not 4.3.x, could this be marked for a 5.0 milestone at a minimum? Thank you.

@rmorshea
Copy link
Contributor

rmorshea commented Jul 22, 2019

No ETA on the next release right now, however I now have access to the PyPI repo so I can help facilitate it. That said, we need to create an issue which compiles the PRs that should be included so @minrk can review. If you have time to help create that issue it would speed up the process a lot.

@rmorshea
Copy link
Contributor

Just noticed that you said 4.3.x - I could try to put together a release with just this PR for 4.3.3. However it would be nice to get help compiling an issue to help get 4.4 out since master has diverged a lot from what’s currently out there.

@kevin-bates
Copy link
Contributor Author

Thanks Ryan. Per my comment above, #341 will be required since that's the change that introduces the tracking of which config files are currently loaded.

I'm happy to help with a release although my knowledge of traitlets is strictly limited to the change I made - so I'm not sure where I can be of assistance.

@rmorshea
Copy link
Contributor

Ok I thought you were more familiar with Traitlets. In that case I wouldn’t worry about compiling PRs for a 4.4 release. I think I just need to take make time on a weekend to just sit down for a 4.4 review session.

Given that, here’s what I need to do and what you can do to get a release with these changes:

  1. There is no 4.3.x branch - I need to make that for you.
  2. Once I’ve made a 4.3.x branch, you can make a PR to it that includes add Application.show_config[_json] #341 and these changes so @minrk can review.
  3. Once we merge it into 4.3.x I can release.

How does that sound?

@kevin-bates
Copy link
Contributor Author

Fantastic - thank you!

@rmorshea
Copy link
Contributor

@kevin-bates added a 4.3.x branch and created this issue to track progress:

#530

@kevin-bates kevin-bates deleted the expose-loaded-config-files branch August 12, 2019 17:37
@Carreau Carreau added this to the 5.0 milestone Jun 4, 2020
@Carreau Carreau added 5.0-re-review Need to re-review for potential API impact changes. 5.0-minor rereviewed, minor change need to be put in changelog. labels Jun 4, 2020
@Carreau Carreau removed the 5.0-re-review Need to re-review for potential API impact changes. label Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0-minor rereviewed, minor change need to be put in changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants