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

feat: add new caching route lambda for async routing invocation #887

Merged
merged 5 commits into from
Oct 31, 2024

Conversation

jsy1218
Copy link
Member

@jsy1218 jsy1218 commented Oct 24, 2024

we want to try creating a new caching routing lambda for async lambda invocation, that will not get exposed through the api gateway. this separates the live path from the async path through two different routing lambdas.

On my local routing dashboard, I tuned all cached routes rollout percent to 100%. I can see the caching intent charts are still there:

Screenshot 2024-10-30 at 4 01 17 PM

I also added a test to assert against the spied metric that hits the new caching routing lambda

Copy link

socket-security bot commented Oct 24, 2024

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@jsy1218 jsy1218 marked this pull request as ready for review October 24, 2024 20:22
@jsy1218 jsy1218 marked this pull request as draft October 28, 2024 17:47
@jsy1218 jsy1218 marked this pull request as ready for review October 28, 2024 17:48
@jsy1218 jsy1218 force-pushed the siyujiang/fix-cached-routes-cache-invalidation branch from e9e4a59 to 0619f26 Compare October 28, 2024 21:18
@jsy1218 jsy1218 force-pushed the siyujiang/new-caching-route-lambda branch 2 times, most recently from 7aa13c4 to 230ba54 Compare October 28, 2024 22:23
@graphite-app graphite-app bot requested a review from a team October 28, 2024 22:40
@graphite-app graphite-app bot requested review from cgkol and xrsv and removed request for a team October 28, 2024 22:40
Copy link

graphite-app bot commented Oct 28, 2024

Graphite Automations

"Request reviewers once CI passes on routing-api repo" took an action on this PR • (10/28/24)

4 reviewers were added and 1 assignee was added to this PR based on 's automation.

Base automatically changed from siyujiang/fix-cached-routes-cache-invalidation to main October 29, 2024 13:29
@jsy1218 jsy1218 force-pushed the siyujiang/new-caching-route-lambda branch from 162119e to 4d9a6e2 Compare October 30, 2024 14:47
Copy link
Collaborator

@cgkol cgkol left a comment

Choose a reason for hiding this comment

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

I think we need a test to confirm that the right lambda is invoked, otherwise we are good

@jsy1218 jsy1218 force-pushed the siyujiang/new-caching-route-lambda branch from 4d9a6e2 to 804d9d3 Compare October 30, 2024 16:25
lib/handlers/injector-sor.ts Outdated Show resolved Hide resolved
@xrsv
Copy link
Contributor

xrsv commented Oct 30, 2024

that's a good starting point - i was thinking if it's possible/easy for the caching lambda to only execute logic related to cache update, instead of the entire handler code. We can discuss offline.

@jsy1218
Copy link
Member Author

jsy1218 commented Oct 30, 2024

yea lets discuss offline, im also thinking about how to write a test that can assert it invokes the caching lambda

@jsy1218 jsy1218 requested review from cgkol and xrsv October 30, 2024 20:15
@jsy1218 jsy1218 merged commit 3d46349 into main Oct 31, 2024
8 checks passed
@jsy1218 jsy1218 deleted the siyujiang/new-caching-route-lambda branch October 31, 2024 19:39
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