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

Cache client state #198

Merged
merged 8 commits into from
Apr 23, 2024
Merged

Cache client state #198

merged 8 commits into from
Apr 23, 2024

Conversation

sfackler
Copy link
Member

@sfackler sfackler commented Mar 24, 2024

Before this PR

Every call to ClientFactory::client created a new, independent client. This prevented connection sharing between clients made for distinct service types for the same remote service.

After this PR

==COMMIT_MSG==
Client state is now cached and shared between clients.
==COMMIT_MSG==

The cache will evict states that are no longer referenced immediately. This matches Dialogue's new behavior as of palantir/dialogue#2194.

Possible downsides?

The cache is currently quite conservative - we could potentially share connections between e.g. clients with differing idempotency settings. An improved setup could be to separate socket-level and other settings and have a separate connection pool cache.

If we add more client settings, there is a risk of forgetting to include them in the cache key. I'm not sure of a good way to avoid that though - maybe storing the CacheKey directly in Builder? The config fields are now explicitly partitioned between cached and uncached.

Closes #45

Base automatically changed from staged-builders to develop March 28, 2024 15:29
@changelog-app
Copy link

changelog-app bot commented Mar 28, 2024

Generate changelog in changelog/@unreleased

What do the change types mean?
  • feature: A new feature of the service.
  • improvement: An incremental improvement in the functionality or operation of the service.
  • fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
  • break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
    and external consumers of the service's API (e.g. customer-written software or integrations).
  • deprecation: Advertises the intention to remove service functionality without any change to the
    operation of the service itself.
  • manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
    performing database surgery, ...) at the time of upgrade for it to succeed.
  • migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?
  • ❗The break and manual task changelog types will result in a major release!
  • 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
  • ✨ All others will result in a minor version release.

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Client state is now cached and shared between clients.

Check the box to generate changelog(s)

  • Generate changelog entry

@sfackler sfackler changed the title [Stacked] Cache client state Cache client state Mar 28, 2024
Comment on lines 44 to 46
pub struct ClientCache {
inner: Arc<Mutex<Inner>>,
}

Choose a reason for hiding this comment

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

What is the purpose of having this inner field on the public struct? Does it hide your Inner cache implementation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep!

TheKeveloper
TheKeveloper previously approved these changes Apr 16, 2024
}

#[derive(Clone)]
pub struct ClientCache {

Choose a reason for hiding this comment

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

It's probably worth adding some tests for the ClientCache logic, especially since the evictor logic is a bit tricky

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I think the best route is probably to make the cache generic so I can use a different type than DefaultRawClient for testing?

Copy link

@TheKeveloper TheKeveloper Apr 16, 2024

Choose a reason for hiding this comment

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

Yeah, that sounds right. If you make the cache key generic it might also make it easier to move that closer to the builder logic, where the builder comes with a method to generate the cache key.

@policy-bot policy-bot bot dismissed TheKeveloper’s stale review April 20, 2024 23:12

Invalidated by push of 680b66a

@sfackler
Copy link
Member Author

Made a few updates

  • Renamed the cache to WeakCache, parameterized it, and wrote a few simple tests.
  • Explicitly partitioned client config into the cacheable and uncacheable sets. This should make it harder to forget to update the cache when adding new config in the future.

pub fn get<T>(
&self,
seed: &T,
get_key: impl FnOnce(&T) -> &K,

Choose a reason for hiding this comment

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

get_key and make_value feel like they should go in new and not on each invocation to get?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but the only reason I genericized the cache was for the tests, and moving the functions into new would make those a bit more awkward to write.

Copy link

@TheKeveloper TheKeveloper left a comment

Choose a reason for hiding this comment

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

Just one small comment on where we place get_key and make_value. I think it would be cleaner to put those once on new and just store them as properties on the cache, kind of like what Caffeine does.

If there's a good reason why that's tough to do, I'm good with merging this as is to unblock.

@sfackler sfackler merged commit f027265 into develop Apr 23, 2024
4 checks passed
@sfackler sfackler deleted the client-cache branch April 23, 2024 22:59
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.

Cache clients in ClientFactory
4 participants