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

add scheduler client (with caching) #1187

Merged
merged 4 commits into from
Dec 10, 2020
Merged

Conversation

cgardens
Copy link
Contributor

@cgardens cgardens commented Dec 3, 2020

This is a low priority PR. Wanted to try something out on caching while I was making changes in the area. This PR does not block anything related to your current weekly or monthly goals

What

How

  • remove spec cache
  • replace it with a scheduler client interface.
  • put caching behind the client interface.

Outcome

  • I am happier with the structure of the code.
  • I still feel like handling cache invalidation isn't maybe as hidden as you had hoped @michel-tricot . Assuming that we need this invalidation, curious if you see something to do better here. We can also just forget about it and have someone restart the server to invalidate the cache.
  • Definitely took 3x longer to implement than the original spec cache. 😛 . It's better and worth it.

Checklist

  • Unit Tests for client

Recommended reading order

  1. SchedulerJobClient
  2. DefaultSchedulerJobClient
  3. CachingSchedulerJobClient.
  4. SpecCachingSchedulerJobClient.
  5. SchedulerHandler
  6. the rest


}
void resetCache();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the part i mentioned in the PR description. caching isn't fully hidden. but couldn't really think of another way to allow the caller to invalidate the cache without exposing it like this.

* case where the docker image is replaced with an image of the same name and tag) and they are
* called very frequently.
*/
public class SpecCachingSchedulerJobClient extends DefaultSchedulerJobClient implements CachingSchedulerJobClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually delegation instead of inheritance for that kind of class:

new SpecCacheSJC(client);

most of the methods would just call:
client.xxx()

and only the one where you want to change behavior you add your code.

That allows you to compose features on top of the DefaultClient

You can argue that this is over-engineered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also since you can not hide the cache invalidation, I guess it doesn't really matter.

Base automatically changed from cgardens/job_attempts_api2 to master December 9, 2020 23:22
@cgardens cgardens merged commit c31d89e into master Dec 10, 2020
@cgardens cgardens deleted the cgardens/scheduler_client branch December 10, 2020 00:43
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.

2 participants