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(schema) Add retry and filter to the root-level of throttle config #796

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

kola-er
Copy link
Contributor

@kola-er kola-er commented Jun 4, 2024

  • Add "retry" and "filter" to the root-level of the throttle configuration
  • Remove throttle for hook trigger.

type: 'boolean',
},
filter: {
description: `EXPERIMENTAL: Account-based attribute to override the throttle by. You can set to one of the following: "free", "trial", "paid". Therefore, the throttle scope would be automatically set to "account" and ONLY the accounts based on the specified filter will have their requests throttled based on the throttle overrides while the rest are throttled based on the original configuration.`,
Copy link
Member

Choose a reason for hiding this comment

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

  1. Can you share why filter is experimental while retry is not?
  2. Regarding "the throttle scope would be automatically set to 'account'", what would happen if scope is not account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding "the throttle scope would be automatically set to 'account'", what would happen if scope is not account?

filter is an account-based config setting that uses ONLY account as the scope and disregards any explicit scope set.

Can you share why filter is experimental while retry is not?

I assumed we wanted to open up the retry to the public due to the need it was requested for.

As we're planning on increasing the polling rate for all integrations, we're concerned about one or two apps which might become rate limited and thus, want to throttle them in advance to avoid issues...

Ultimately I think we need to avoid the possibility of stacking up retry attempts faster than they'll actually be processed, causing a pile that only gets bigger with time...

from https://zapier.slack.com/archives/C04P5J5R8E4/p1716513877072559

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FokkeZB Please, did you intend to make the root-level retry experimental when you suggested it? https://zapier.slack.com/archives/D3YRTGHAM/p1717433047352299

Copy link
Contributor

Choose a reason for hiding this comment

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

What's effect of marking it as experimental? Does that imply it's only for internal usage? As the override-level filter property isn't experimental either, I don't see why the root-level one should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FokkeZB Yes, experimental implies internal usage. The override-level filter is experimental indirectly because the override object itself is experimental.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok. I don't have a strong opinion on whether this one should be experimental.

eliangcs
eliangcs previously approved these changes Jun 6, 2024
@kola-er
Copy link
Contributor Author

kola-er commented Jun 7, 2024

@eliangcs Documented and added a functional constraint test on preventing devs from setting the retry field for polling triggers. Since there's no point in retrying polling tasks, the backend ignores it. Please, review the latest commit. Thanks.

Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

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

Looks good except for the suggestion below. I'll approve once that's fixed.

Question: Why can't a polling trigger have retry? Does that mean it makes sense for a hook trigger to have retry?

@@ -83,7 +83,6 @@ BasicHookOperationSchema.properties = {
inputFields: BasicHookOperationSchema.properties.inputFields,
outputFields: BasicHookOperationSchema.properties.outputFields,
sample: BasicHookOperationSchema.properties.sample,
throttle: BasicHookOperationSchema.properties.throttle,
Copy link
Member

Choose a reason for hiding this comment

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

👍

@kola-er kola-er merged commit c4b97cd into main Jun 12, 2024
13 checks passed
@kola-er kola-er deleted the PDE-5072 branch June 12, 2024 10:05
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.

None yet

3 participants