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

[WIP] JobTypeLoader Enhancement #334

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

[WIP] JobTypeLoader Enhancement #334

wants to merge 22 commits into from

Conversation

opalmer
Copy link
Member

@opalmer opalmer commented Sep 27, 2015

This PR provides a few bugfixes, changes and improvements to the way we're loading job types. Overall the code is simpler than the old setup and more thoroughly tested as well. Here's a complete list of changes/enhancements coming out of this PR over the old design:

Bugfixes

  • If a job type was removed from the master between the time we received work and when we started, the download request will not fail. Before continue to try and process the result anyway.
  • We now explicitly handle individual http codes from the master. Before the code assumed any http response >= 500 should be retried and everything else was ok.
  • Properly handle cases where a job type's class name could be None
  • Caching will now ignore certain errors, such as disk space or permission issues, so we can continue to process a job type rater than fail in the loading process. It's possible the job type could cleanup disk space or not require space where we're writing the cache so it shouldn't prevent the job type from running. Raising errors outside of job type execution also didn't give an implementor a lot of control or a chance to notify the master of problems during executing.

Refactors

  • Cache has been renamed to JobTypeLoader. This new name better fits the intended function and is something that is intended to perform the entire process of retrieving and returning a job type rather than mostly just caching.
  • JobTypeLoader is no longer a parent class of JobType. Instead it's instanced as the class variable JobType.LOADER. This simplifies the overall class structure some, puts fewer inherited methods under each JobType instance and also allows a JobType implementor (or tests) to add their own loader more easily.

Enhancements

  • JobType.load() has been simplified. Now it only validates the schema then hands off the whole loading process to JobTypeLoader.
  • Caching is disk based only now. We were not getting many performance benefits with disk based and ram based caching. The old setup was also more complicated than it probably needed to be too. This should also reduce the possibility of chewing through extra ram for larger job types.
  • Unlike Cache, JobTypeLoader can be tested and used independently of a JobType instance more easily.

Unfortunately I don't see a good way to break this PR down further. Unlike previous attempts that patched the existing Cache class this one replaces it instead with simpler code.

@opalmer opalmer self-assigned this Sep 27, 2015
@opalmer opalmer added this to the 0.8.7 milestone Sep 27, 2015
else:
logger.info("Created job type cache directory %r", CACHE_DIRECTORY)
try:
cache_directory = config["jobtype_cache_directory"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we join config['farm_name'] to this, if it is set?

That way, if agents get switched between farms for some reason, they won't get the different jobtypes caches mixed up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes fixed, good catch thanks.

@opalmer
Copy link
Member Author

opalmer commented Oct 4, 2015

Still a work in progress, there are either some issues with the TestAssign.test_accepted test or something in Assign.post itself is not playing well with inlineCallbacks. Right now my theory is that because test_accepted does not wait for a result and is more or less executed in a linear fashion not allowing JobTypeLoader to finish the work it needs before the test finishes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants