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: Let services self-evict #456

Merged
merged 1 commit into from
Mar 10, 2020
Merged

cache: Let services self-evict #456

merged 1 commit into from
Mar 10, 2020

Conversation

olix0r
Copy link
Member

@olix0r olix0r commented Mar 6, 2020

This change modifies the cache crate to remove its background task
which is responsible for tracking service use and evicting idle
services.

Now, the cache (1) has no capacity limit and (2) evicts services from
the cache when they drop their underlying service.

This is accomplished by modifying the CacheLayer to accept a layer
that is responsible for making the inner service shareable and for
dropping its inner service when it should be considered defunct. This is
accomplished by moving an Arc<()> into the inner stack and holding a
Weak<()> reference to it in the cache module. Then, before a request
is processed by the cache, it attempts to upgrade each cache entry and
drops services that have dropped the inner service (holding the strong
reference).

@olix0r olix0r requested a review from a team March 6, 2020 21:05
linkerd/app/inbound/src/lib.rs Outdated Show resolved Hide resolved
linkerd/app/inbound/src/lib.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Contributor

hawkw commented Mar 6, 2020

Testing this branch vs b61b044, it seems like the p99.9 latency regression I observed on #453 now only exists in the outbound direction:
ver-cache-latency

Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

overall, this LGTM, though i definitely agree we should figure out what's up with the test failure before moving forwards with it.

linkerd/app/inbound/src/lib.rs Outdated Show resolved Hide resolved
linkerd/app/outbound/src/lib.rs Show resolved Hide resolved
linkerd/cache/src/lib.rs Show resolved Hide resolved
linkerd/cache/src/lib.rs Outdated Show resolved Hide resolved
This change modifies the `cache` crate to remove its background task
which is responsible for tracking service use and evicting idle
services.

Now, the cache (1) has no capacity limit and (2) evicts services from
the cache when they drop their underlying service.

This is accomplished by modifying the `CacheLayer` to accept a layer
that is responsible for making the inner service shareable and for
dropping its inner service when it should be considered defunct. This is
accomplished by moving an `Arc<()>` into the inner stack and holding a
`Weak<()>` reference to it in the cache module. Then, before a request
is processed by the cache, it attempts to upgrade each cache entry and
drops services that have dropped the inner service (holding the strong
reference).
@olix0r olix0r force-pushed the ver/cache-and-buffer branch from 1c5a3c3 to 5fb6942 Compare March 10, 2020 17:38
@olix0r olix0r marked this pull request as ready for review March 10, 2020 17:38
@olix0r olix0r requested review from hawkw and a team March 10, 2020 17:38
Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@kleimkuhler kleimkuhler left a comment

Choose a reason for hiding this comment

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

Looks good!

@olix0r olix0r merged commit 339efa8 into master Mar 10, 2020
@olix0r olix0r deleted the ver/cache-and-buffer branch March 10, 2020 19:06
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Mar 10, 2020
This release builds on changes in the prior release to ensure that
balancers process updates eagerly.

Cache capacity limitations have been removed; and services now fail
eagerly, rather than making all requests wait for the timeout to expire.

Also, a bug was fixed in the way the `LINKERD2_PROXY_LOG` env variable
is parsed.

---

* Introduce a backpressure-propagating buffer (linkerd/linkerd2-proxy#451)
* trace: update tracing-subscriber to 0.2.3 (linkerd/linkerd2-proxy#455)
* timeout: Introduce FailFast, Idle, and Probe middlewares (linkerd/linkerd2-proxy#452)
* cache: Let services self-evict (linkerd/linkerd2-proxy#456)
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Mar 10, 2020
This release builds on changes in the prior release to ensure that
balancers process updates eagerly.

Cache capacity limitations have been removed; and services now fail
eagerly, rather than making all requests wait for the timeout to expire.

Also, a bug was fixed in the way the `LINKERD2_PROXY_LOG` env variable
is parsed.

---

* Introduce a backpressure-propagating buffer (linkerd/linkerd2-proxy#451)
* trace: update tracing-subscriber to 0.2.3 (linkerd/linkerd2-proxy#455)
* timeout: Introduce FailFast, Idle, and Probe middlewares (linkerd/linkerd2-proxy#452)
* cache: Let services self-evict (linkerd/linkerd2-proxy#456)
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