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

Account settings via HttpPeriodicRefreshService #1665

Open
jbartek25 opened this issue Jan 3, 2022 · 2 comments
Open

Account settings via HttpPeriodicRefreshService #1665

jbartek25 opened this issue Jan 3, 2022 · 2 comments

Comments

@jbartek25
Copy link
Contributor

Hey guys,

We are using a HTTP service for fetching accounts, requests and imps. This works fine when using HttpApplicationSettings but requires us to either periodically call the config services on predefined TTL intervals or the backend logic has to call the cache invalidation endpoint on each change.

Alternative approach is to use HttpPeriodicRefreshService which downloads all the config objects at the startup and then only fetches changed items on predefined interval. We like this approach more than what HttpApplicationSettings does but the problem is that HttpPeriodicRefreshService works only with requests and imps, not accounts. We could use HttpApplicationSettings for accounts and HttpPeriodicRefreshService for requests and imps but the downside of the approach is that we would need to

  • either set the cache TTL sufficiently low to keep account settings fresh (the TTL is shared by requests and imps leading to their invalidation and reload cutting the benefits of using HttpPeriodicRefreshService)
  • or we would have to start calling invalidation endpoint just for accounts

We looked at the code and as we see it, HttpPeriodicRefreshService cannot be easily extended to also fetch accounts, because HttpPeriodicRefreshService uses CacheNotificationListener which allows updating the cache directly but only supports requests and imps and it seems a rather large change to get accounts use the same caching mechanism as requests and imps.

Questions:

  1. Is there a plan in the near future to add support for fetching accounts via HttpPeriodicRefreshService?
  2. If answer on 1. is "no", we are testing a simplified implementation that we are happy to contribute: accounts would be fetched in HttpPeriodicRefreshService but instead of updating the account cache directly which is currently not possible, the cache of updated accounts would simply get invalidated and hence reloaded by HttpApplicationSettings (or any other fetch method configured).

Thanks & Regards

@bretg
Copy link
Collaborator

bretg commented Jul 11, 2022

Thanks for the detailed enhancement description @jbartek25 . For the record, I only reviewed the main PBS repo for issues. Will start looking here every two weeks in prep for the committee meeting.

Anyhow, as is apparent from the lack of response, no, there is no plan to change the fetching of accounts. Community contributions are always welcome, particularly if they're configurable.

We have discussed possibly adding a mode where the server can load a large batch of account data into cache at startup before accepting requests.

@bretg bretg pinned this issue Sep 6, 2022
@bretg bretg unpinned this issue Sep 6, 2022
@bretg
Copy link
Collaborator

bretg commented Mar 24, 2023

@jbartek25 - discussed in committee -- we're open to contributions like this.

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

No branches or pull requests

2 participants