Add probes cache #1401

pierDipi · 2021-11-03T06:51:28Z

This patch adds a probe cache that allows to associate a Status
to a given key.

The cache prunes expired entries and calls a callback when
an entry is expired, so that controllers can re-queue associated
resources.

This is part of our work to reduce e2e tests' flakiness.

Proposed Changes

Add probes cache

Release Note

None

Docs

None

This patch adds a probe cache that allows to associate a `Status` to a given key. The cache prunes expired entries and calls a callback when an entry is expired, so that controllers can re-queue associated resources. Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>

codecov · 2021-11-03T06:55:40Z

Codecov Report

Merging #1401 (ef9684e) into main (c73237e) will decrease coverage by 4.90%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1401      +/-   ##
============================================
- Coverage     76.57%   71.66%   -4.91%     
  Complexity      584      584              
============================================
  Files            99      101       +2     
  Lines          3607     3999     +392     
  Branches        165      166       +1     
============================================
+ Hits           2762     2866     +104     
- Misses          635      915     +280     
- Partials        210      218       +8

Flag	Coverage Δ
java-unittests	`82.28% <ø> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
control-plane/pkg/prober/cache.go	`100.00% <100.00%> (ø)`
control-plane/pkg/reconciler/source/controller.go	`72.34% <0.00%> (-27.66%)`	⬇️
control-plane/pkg/reconciler/kafka/topic.go	`97.67% <0.00%> (-2.33%)`	⬇️
control-plane/pkg/core/config/resource.go	`100.00% <0.00%> (ø)`
control-plane/pkg/reconciler/sink/kafka_sink.go	`67.83% <0.00%> (ø)`
...lane/pkg/reconciler/base/receiver_condition_set.go	`0.00% <0.00%> (ø)`
control-plane/pkg/config/env_config.go
control-plane/pkg/reconciler/channel/controller.go	`85.71% <0.00%> (ø)`
control-plane/pkg/reconciler/channel/channel.go	`0.00% <0.00%> (ø)`
control-plane/pkg/reconciler/broker/broker.go	`70.44% <0.00%> (+0.14%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c73237e...ef9684e. Read the comment docs.

aliok · 2021-11-08T13:13:34Z

Looks good, but where's this gonna be used? Like, there's no integration right now

matzew · 2021-11-08T13:14:26Z

control-plane/pkg/prober/cache.go

+	GetStatus(key string) Status
+	// UpsertStatus add or updates the status associated with the given key.
+	// Once the given key expires the onExpired callback will be called passing the arg parameter.
+	UpsertStatus(key string, status Status, arg interface{}, onExpired ExpiredFunc)


How about something like PutIfAbsent ? 😄

Should it be just put?
This is doing a replace

PutStatus works for me, even though I think UpsertStatus conveys better the idea of adding or replacing something.

matzew · 2021-11-08T13:15:57Z

control-plane/pkg/prober/cache.go

+	onExpired  ExpiredFunc
+}
+
+func NewInMemoryLocalCache(ctx context.Context, expireDuration time.Duration) Cache {


expireDuration -> expiration ?

matzew · 2021-11-08T13:16:27Z

control-plane/pkg/prober/cache.go

+	// This allows fast deletion of expired entries.
+	entries *list.List
+
+	expireDuration time.Duration


expireDuration -> expiration ?

isn't that typically called ttl ?

not typically. I like expiration over ttl. I think expiring is stronger than ttl

(heavily biased based on heavy usage of a sweet library like https://github.com/jhalterman/expiringmap - which this PR is bascially also trying to do)

I'm using expiration, for now, I think we all get the meaning of this variable with that name.

matzew · 2021-11-08T13:16:49Z

control-plane/pkg/prober/cache.go

+}
+
+// ExpiredFunc is a callback called once an entry in the cache is expired.
+type ExpiredFunc func(key string, arg interface{})


ExpirationFunc?

matzew · 2021-11-08T13:18:43Z

control-plane/pkg/prober/cache.go

+// ExpiredFunc is a callback called once an entry in the cache is expired.
+type ExpiredFunc func(key string, arg interface{})
+
+type inMemoryLocalCache struct {


I'd call that different, and express that the cache has some built-in expiration

localExpiringCache ?

Do we really need the "inMemory" in the name? If so: inMemoryLocalExpiringCache ?

Done, localExpiringCache

pierDipi · 2021-11-09T06:31:57Z

Looks good, but where's this gonna be used? Like, there's no integration right now

Integration in a separate PR.

This cache will be added as a reconciler field and used in ReconcilerKind and FinalizeKind.

Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>

knative-metrics-robot · 2021-11-09T07:39:53Z

The following is the coverage report on the affected files.
Say /test pull-knative-sandbox-eventing-kafka-broker-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
control-plane/pkg/prober/cache.go	Do not exist	96.9%

matzew

/lgtm
/approve

thanks for addressing the comments.

Perhaps even worth to extract as a lib :-)

knative-prow-robot · 2021-11-09T16:43:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, pierDipi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [matzew,pierDipi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pierDipi requested review from matzew, aliok and devguyio November 3, 2021 06:51

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 3, 2021

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/control-plane labels Nov 3, 2021

matzew reviewed Nov 8, 2021

View reviewed changes

Rename using expiration and local

ef9684e

Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>

pierDipi force-pushed the SRVKE-957 branch from 508a1b6 to ef9684e Compare November 9, 2021 07:37

pierDipi requested a review from matzew November 9, 2021 07:43

matzew approved these changes Nov 9, 2021

View reviewed changes

knative-prow-robot assigned matzew Nov 9, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 9, 2021

knative-prow-robot merged commit 67f0993 into knative-extensions:main Nov 9, 2021

pierDipi deleted the SRVKE-957 branch November 10, 2021 10:51

pierDipi mentioned this pull request Nov 18, 2021

Probe resource ingress for readiness #1495

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add probes cache #1401

Add probes cache #1401

pierDipi commented Nov 3, 2021

codecov bot commented Nov 3, 2021 •

edited

aliok commented Nov 8, 2021

matzew Nov 8, 2021

pierDipi Nov 9, 2021

pierDipi Nov 9, 2021

matzew Nov 8, 2021

pierDipi Nov 9, 2021

matzew Nov 8, 2021

devguyio Nov 8, 2021

matzew Nov 8, 2021

pierDipi Nov 9, 2021

matzew Nov 8, 2021

pierDipi Nov 9, 2021

matzew Nov 8, 2021

pierDipi Nov 9, 2021

pierDipi commented Nov 9, 2021

knative-metrics-robot commented Nov 9, 2021

matzew left a comment

knative-prow-robot commented Nov 9, 2021

Add probes cache #1401

Add probes cache #1401

Conversation

pierDipi commented Nov 3, 2021

Proposed Changes

codecov bot commented Nov 3, 2021 • edited

Codecov Report

aliok commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierDipi commented Nov 9, 2021

knative-metrics-robot commented Nov 9, 2021

matzew left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Nov 9, 2021

codecov bot commented Nov 3, 2021 •

edited