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

Stop ol-solr-updater from spinning #281

Merged
merged 2 commits into from
Mar 24, 2017
Merged

Stop ol-solr-updater from spinning #281

merged 2 commits into from
Mar 24, 2017

Conversation

cpeel
Copy link
Contributor

@cpeel cpeel commented Dec 31, 2015

Since ia_db isn't defined in the config, update_work.py throws an exception causing upstart to continually respawn ol-solr-updater resulting in CPU thrashing as the process continuously respawns.

Solve the respawning problem by checking to see if the ia_db key exists in the first place, and protect against future respawn-storms with a respawn limit. This allows ol-solr-updater to start, although I don't have any idea what it does functionally to it (ie: does it still work in the devel vagrant image or just die somewhere else?).

new-solr-updater.py should never require respawning, but if it does
because of an error, don't get into a massive respawn spin cycle.
If the process respawns 10 times within 5 seconds, stop trying.
The ia_db configuration isn't a part of the vagrant devel config
and causes a traceback. This allows the ol-solr-updater upstart
job to successfully start.
@jack17529
Copy link
Contributor

@cpeel are you working on this or should I take the issue?

@cpeel
Copy link
Contributor Author

cpeel commented Mar 23, 2017

@jack17529 - I am not working on this, feel free to take it.

Copy link
Contributor

@jack17529 jack17529 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can anybody with write access can show me conflicts?
what he did was right .

@@ -6,5 +6,6 @@ stop on runlevel [!2345]

chdir /openlibrary
respawn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the first respawn be removed (replaced with the second respawn limit 10 5)? Or should both be present

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mekarpeles how much do you want to respawn the default is set as
Default COUNT is 10. Default INTERVAL is 5 seconds.
If you want to respawn many times use -
respawn limit unlimited

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want both. The first says to respawn, the second sets a limit. See: http://upstart.ubuntu.com/cookbook/#respawn and http://upstart.ubuntu.com/cookbook/#respawn-limit

@@ -1789,7 +1789,8 @@ def load_configs(c_host,c_config,c_data_provider):
conf_file = c_config

global _ia_db
_ia_db = get_ia_db(config.runtime_config['ia_db'])
if ('ia_db' in config.runtime_config.keys()):
_ia_db = get_ia_db(config.runtime_config['ia_db'])

global data_provider
if data_provider == None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably change this to is None while we're at it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh did not notice that PEP 8 says to use is None instead of None.

@mekarpeles
Copy link
Member

I believe the changes just need to be rebased against master and the tests will likely pass

@mekarpeles
Copy link
Member

Re-ran CI and no problems -- @cpeel can offer feedback on #281 (review)? If so I'm happy to merge this in. Thanks for pursuing this @jack17529.

@mekarpeles mekarpeles merged commit 79a4f5c into internetarchive:master Mar 24, 2017
@mekarpeles
Copy link
Member

Merged, thanks @cpeel and @jack17529! 🙇 🙇

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