[FUN-973] s4 snapshot caching #12275

agparadiso · 2024-03-04T14:39:41Z

Description

This pr introduces a s4 orm cache wrapper that accepts a ORM interface and wraps it with a in memory cache layer.
At the moment we are only focusing on caching the retrieve of Snapshots but the rest of the methods can also be cached if needed.

the Update method will invalidate the cache only if the given address is within the range of address cached.

github-actions · 2024-03-04T14:42:39Z

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

bolekk

looks reasonable!

bolekk · 2024-03-06T06:01:40Z

core/services/s4/cached_orm_wrapper.go

+	return c.underlayingORM.GetUnconfirmedRows(limit, qopts...)
+}
+
+func (c CachedORM) clearCache(row *Row) error {


I'm not sure if returning an error from this method has any value. If any of the three error cases below happen, we should simply delete the key and continue iterating in the loop.

makes sense! added the changes 👍🏼

bolekk · 2024-03-06T06:02:54Z

core/services/s4/cached_orm_wrapper.go

+}
+
+func (c CachedORM) DeleteExpired(limit uint, utcNow time.Time, qopts ...pg.QOpt) (int64, error) {
+	return c.underlayingORM.DeleteExpired(limit, utcNow, qopts...)


I wonder if we should clear the cache here if DeleteExpired removed any entries.

💡 good idea, I'll flush the cache to start fresh given there is no way of knowing what was deleted

bolekk · 2024-03-06T16:00:48Z

core/services/s4/cached_orm_wrapper.go

+	for key := range c.cache.Items() {
+		keyParts := strings.Split(key, "_")
+		if len(keyParts) != 3 {
+			c.lggr.Errorf("invalid cache key: %s", key)


Still delete the key on every error? We don't want any invalid keys in the cache anyway.

got it, I've addressed it on this commit.

I changed it slightly to delete only if the key is a GetSnapshot key and it fails to parse the addresses.
I think this is safer, because if at some point we decide to add a cache layer to the eg the Get(address *ubig.Big, slotId uint, qopts ...pg.QOpt) (*Row, error) this will wrongly delete records and it will be painful to find why.
Worst case scenario if the key is wrong it will die because of the TTL, wdyt?

KuphJr · 2024-03-06T20:45:37Z

core/services/s4/cached_orm_wrapper.go

+	"strings"
+	"time"
+
+	"github.com/patrickmn/go-cache"


This dependency must already exist I suppose since I don't see any changes to the go.mod file

yes! the evmregistry is already using it

KuphJr · 2024-03-06T20:46:19Z

core/services/s4/cached_orm_wrapper.go

+)
+
+// CachedORM is a cached orm wrapper that implements the ORM interface.
+// It adds a cache layer in order to remove unnecessary preassure to the underlaying implementation


nit: spelling of pressure

KuphJr · 2024-03-06T21:11:44Z

core/services/s4/cached_orm_wrapper.go

+			continue
+		}
+
+		if row.Address.ToInt().Cmp(minAddress) >= 0 && row.Address.ToInt().Cmp(maxAddress) <= 0 {


I am trying to understand a case where this would not be true...

S4 supports snapshot sharding.

Gotcha. I re-read the CLIP and this now makes sense.

yes, we want to only delete the cached snapshots that its addressRange contain a specific row. Just to avoid deleting snapshots that are not affected

KuphJr · 2024-03-06T21:48:56Z

core/services/s4/cached_orm_wrapper.go

+		return cached.([]*SnapshotRow), nil
+	}
+
+	c.lggr.Info("Snapshot not found in cache, fetching it from underlaying implementation")


Nit: this could probably be debug level

KuphJr · 2024-03-06T21:55:24Z

core/services/s4/cached_orm_wrapper.go

+	}
+
+	if deletedRows > 0 {
+		c.cache.Flush()


Why do we need to flush the entire cache every time a single row is expired?

more ctx here but basically its because we know some rows where deleted, but we don't know which ones, so the best effort on keeping the cache sync is to flush it

cl-sonarqube-production · 2024-03-11T11:48:55Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
78.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

* feat: s4 get snapshot cached * chore: log and continue in case of error cleaning the cache * feat: flush cache in case of deleting expired from underlaying orm * feat: delete invalid keys * chore: typo + log level

agparadiso force-pushed the feat/FUN-973_s4_snapshot_caching branch from 8d415c1 to 4ca8dd6 Compare March 4, 2024 14:42

agparadiso had a problem deploying to sdlc March 4, 2024 14:42 — with GitHub Actions Failure

agparadiso force-pushed the feat/FUN-973_s4_snapshot_caching branch from 4ca8dd6 to 051cd9e Compare March 4, 2024 15:26

agparadiso had a problem deploying to sdlc March 4, 2024 15:27 — with GitHub Actions Failure

agparadiso force-pushed the feat/FUN-973_s4_snapshot_caching branch from 051cd9e to 1652ecd Compare March 4, 2024 16:04

agparadiso temporarily deployed to sdlc March 4, 2024 16:05 — with GitHub Actions Inactive

agparadiso force-pushed the feat/FUN-973_s4_snapshot_caching branch from 1652ecd to a025fbc Compare March 4, 2024 17:11

agparadiso temporarily deployed to sdlc March 4, 2024 17:11 — with GitHub Actions Inactive

agparadiso force-pushed the feat/FUN-973_s4_snapshot_caching branch from a025fbc to 7af9645 Compare March 4, 2024 17:13

agparadiso temporarily deployed to sdlc March 4, 2024 17:13 — with GitHub Actions Inactive

agparadiso force-pushed the feat/FUN-973_s4_snapshot_caching branch from 7af9645 to d0fe543 Compare March 5, 2024 12:14

agparadiso temporarily deployed to sdlc March 5, 2024 12:14 — with GitHub Actions Inactive

agparadiso force-pushed the feat/FUN-973_s4_snapshot_caching branch from d0fe543 to 3beb238 Compare March 5, 2024 12:18

agparadiso temporarily deployed to sdlc March 5, 2024 12:18 — with GitHub Actions Inactive

feat: s4 get snapshot cached

29a1558

agparadiso force-pushed the feat/FUN-973_s4_snapshot_caching branch from 3beb238 to 29a1558 Compare March 5, 2024 13:06

agparadiso temporarily deployed to sdlc March 5, 2024 13:06 — with GitHub Actions Inactive

agparadiso marked this pull request as ready for review March 5, 2024 13:32

agparadiso requested a review from a team as a code owner March 5, 2024 13:32

agparadiso self-assigned this Mar 5, 2024

bolekk reviewed Mar 6, 2024

View reviewed changes

agparadiso added 2 commits March 6, 2024 15:30

chore: log and continue in case of error cleaning the cache

c676b85

feat: flush cache in case of deleting expired from underlaying orm

3339fac

agparadiso temporarily deployed to sdlc March 6, 2024 14:47 — with GitHub Actions Inactive

Merge branch 'develop' into feat/FUN-973_s4_snapshot_caching

d17f14e

agparadiso temporarily deployed to sdlc March 6, 2024 14:48 — with GitHub Actions Inactive

agparadiso requested a review from bolekk March 6, 2024 14:49

bolekk reviewed Mar 6, 2024

View reviewed changes

feat: delete invalid keys

2d09526

agparadiso temporarily deployed to sdlc March 6, 2024 16:39 — with GitHub Actions Inactive

agparadiso requested a review from bolekk March 6, 2024 17:16

bolekk previously approved these changes Mar 6, 2024

View reviewed changes

KuphJr previously approved these changes Mar 6, 2024

View reviewed changes

KuphJr reviewed Mar 6, 2024

View reviewed changes

chore: typo + log level

f2b3e96

agparadiso dismissed stale reviews from KuphJr and bolekk via f2b3e96 March 7, 2024 11:50

agparadiso temporarily deployed to sdlc March 7, 2024 11:50 — with GitHub Actions Inactive

Merge branch 'develop' into feat/FUN-973_s4_snapshot_caching

0c5752f

agparadiso temporarily deployed to sdlc March 7, 2024 12:41 — with GitHub Actions Inactive

agparadiso requested a review from KuphJr March 7, 2024 12:41

Merge branch 'develop' into feat/FUN-973_s4_snapshot_caching

72d0822

agparadiso temporarily deployed to sdlc March 7, 2024 15:48 — with GitHub Actions Inactive

KuphJr approved these changes Mar 7, 2024

View reviewed changes

Merge branch 'develop' into feat/FUN-973_s4_snapshot_caching

5fd736c

agparadiso temporarily deployed to sdlc March 7, 2024 17:24 — with GitHub Actions Inactive

Merge branch 'develop' into feat/FUN-973_s4_snapshot_caching

dd93e2a

agparadiso temporarily deployed to sdlc March 11, 2024 11:35 — with GitHub Actions Inactive

agparadiso enabled auto-merge March 11, 2024 11:35

agparadiso added this pull request to the merge queue Mar 11, 2024

Merged via the queue into develop with commit f077a43 Mar 11, 2024
96 of 97 checks passed

agparadiso deleted the feat/FUN-973_s4_snapshot_caching branch March 11, 2024 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FUN-973] s4 snapshot caching #12275

[FUN-973] s4 snapshot caching #12275

agparadiso commented Mar 4, 2024 •

edited

Loading

github-actions bot commented Mar 4, 2024

bolekk left a comment

bolekk Mar 6, 2024

agparadiso Mar 6, 2024

bolekk Mar 6, 2024

agparadiso Mar 6, 2024

bolekk Mar 6, 2024

agparadiso Mar 6, 2024 •

edited

Loading

KuphJr Mar 6, 2024

agparadiso Mar 7, 2024

KuphJr Mar 6, 2024

KuphJr Mar 6, 2024

bolekk Mar 6, 2024

KuphJr Mar 6, 2024

agparadiso Mar 7, 2024

KuphJr Mar 6, 2024

KuphJr Mar 6, 2024

agparadiso Mar 7, 2024

cl-sonarqube-production bot commented Mar 11, 2024

[FUN-973] s4 snapshot caching #12275

[FUN-973] s4 snapshot caching #12275

Conversation

agparadiso commented Mar 4, 2024 • edited Loading

Description

github-actions bot commented Mar 4, 2024

bolekk left a 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

agparadiso Mar 6, 2024 • edited Loading

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

cl-sonarqube-production bot commented Mar 11, 2024

Quality Gate passed

agparadiso commented Mar 4, 2024 •

edited

Loading

agparadiso Mar 6, 2024 •

edited

Loading