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 ADR 005: Publishing API debouncing #230

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add ADR 005: Publishing API debouncing #230

wants to merge 1 commit into from

Conversation

csutter
Copy link
Contributor

@csutter csutter commented Mar 5, 2024

No description provided.

Copy link
Contributor

@leenagupte leenagupte left a comment

Choose a reason for hiding this comment

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

I've left one inline comment.

It's annoying that we have to do this, but this does seem like the simplest option to stop hammering Discovery Engine.

docs/adr/005-publishing-api-debounce.md Outdated Show resolved Hide resolved
Comment on lines +86 to +95
Upon receiving a message, a worker would:
- use the [`redlock-rb` gem][redlock-gem] to acquire a lock of `lock:<content_id>` with a sensible
timeout
- if the lock currently exists, [wait for it to be available again](#addendum-how-to-wait-for-lock)
- check `latest:<content_id>` to see if a newer message has recently been successfully processed
- if so, do nothing and go straight to releasing the lock
- process the message (update or delete the document in Discovery Engine)
- if processing fails, handle as usual (requeue or discard)
- otherwise use `SET latest:<content_id> <version>` to update the latest processed version
- release the lock
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there still a scenario here where we could exceed Discovery Engine's rate
limit if the messages come in quickly in the correct order? Say:

Time(ms) Payload Version Content ID
0 0 cafebabe
10 1 cafebabe
20 2 cafebabe
30 3 cafebabe
40 4 cafebabe
50 5 cafebabe

If each of thos messages gets processed quickly enough, we'll hit the rate limit.

Do we want to put some kind of deliberate delay in to handle that case?

For example, if we made the locks timeout after 1 second, and then just didn't
release them explicitly, that would throttle messages to one message per
second.

If we do release the locks explicitly, then this won't necessarily slow down
the processing of sequential messages by much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout - with the caveat that during peak times latency for an upsert API call to Vertex is about 1s anyway, and only the parallel requests are a problem. But an enforced delay of sorts would probably still help us during off-peak times when it's 0.1s.

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