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

Add 1s cache for lapi.GetStreamByPlaybackID() #1339

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Conversation

leszko
Copy link
Contributor

@leszko leszko commented Jul 23, 2024

It causes the prod failure, because of the cascading failures

Related Discord thread: https://discord.com/channels/423160867534929930/1265176750544130068/1265181494763327562

It causes the prod failure, because of the cascading failures
@leszko leszko requested review from thomshutt and gioelecerati July 23, 2024 12:41
@thomshutt thomshutt requested a review from victorges July 23, 2024 13:07
Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM

"time"
)

const lapiCacheDuration = 1 * time.Second
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the latest version of a stream for any config that can be changed? Maybe this cache could be even higher

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. I'm just afraid to set it higher (and 1s is enough for what we encountered)

stream, err := a.lapi.GetStreamByPlaybackID(playbackId)
a.streamCache[playbackId] = entry{
stream: stream,
err: err,
Copy link
Member

Choose a reason for hiding this comment

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

do we need to cache error cases if we only use cache when it's successful? Could be clearer to have an early return for err != nil here instead and skip the err in cache

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do want to cache errors as well. That was actually the root cause of the issue that we never cached failed requests so they piled up.

@leszko leszko merged commit 1351b0f into main Jul 25, 2024
11 checks passed
@leszko leszko deleted the rafal/add-cache-for-lapi branch July 25, 2024 14:43
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.

2 participants