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: increase courier watchMessages polling delay from 1 second to 30 #3460

Closed
wants to merge 1 commit into from
Closed

feat: increase courier watchMessages polling delay from 1 second to 30 #3460

wants to merge 1 commit into from

Conversation

frederikhors
Copy link
Contributor

@frederikhors frederikhors commented Aug 29, 2023

The database is flooded with calls all the time for this!

Related to #466.

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

The database is flooded with calls all the time for this!
@frederikhors frederikhors changed the title Increase courier watchMessages polling delay from 1 second to 30 fix: Increase courier watchMessages polling delay from 1 second to 30 Aug 29, 2023
@frederikhors frederikhors changed the title fix: Increase courier watchMessages polling delay from 1 second to 30 fix: increase courier watchMessages polling delay from 1 second to 30 Aug 29, 2023
@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Merging #3460 (3a3b260) into master (37f1657) will not change coverage.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 3a3b260 differs from pull request most recent head 173793a. Consider uploading reports for the commit 173793a to get more accurate results

@@           Coverage Diff           @@
##           master    #3460   +/-   ##
=======================================
  Coverage   78.14%   78.14%           
=======================================
  Files         327      327           
  Lines       21742    21742           
=======================================
  Hits        16991    16991           
  Misses       3492     3492           
  Partials     1259     1259           
Files Changed Coverage Δ
courier/courier.go 71.42% <100.00%> (ø)

@frederikhors frederikhors changed the title fix: increase courier watchMessages polling delay from 1 second to 30 feat: increase courier watchMessages polling delay from 1 second to 30 Aug 29, 2023
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @frederikhors

I don't think this is a good solution though. It will take up to 30 seconds + email send delay, before the email arrives at the user. So the user has to wait half a minute before they receive their account recovery code, or their login code. That's unfortunately not acceptable and we would like to keep the watcher at one second for the time being.

@frederikhors
Copy link
Contributor Author

Ok. I get that but I think 30 seconds is a decent wait time for anyone.

What about 15?

@jonas-jonas
Copy link
Member

Maybe, instead of using a hard-coded value, we can make this configurable, by adding an optional key courier.dispatch-interval, that defaults to 1s?

@aeneasr
Copy link
Member

aeneasr commented Sep 8, 2023

Telling the truth, I think this should just stay as is. Adding one QPS to the database should not really have a big effect on overall query performance. I also don't want this to be configurable in Ory Network.

@frederikhors
Copy link
Contributor Author

I think the suggestion to make it configurable is good. Please. Please.

@aeneasr
Copy link
Member

aeneasr commented Sep 8, 2023

What exactly is the problem you are trying to solve with this? Is there a negative effect you are experiencing?

If configurable, it should be at most a CLI flag, but not a configuration setting in the YAML config.

@frederikhors
Copy link
Contributor Author

Is there a negative effect you are experiencing?

Yep. A very small DB (many DBS) and no need to send courier emails.

If you prefer to use a CLI flag ok. Thanks anyway.

@aeneasr
Copy link
Member

aeneasr commented Sep 8, 2023

If you don’t need to send emails from Kratos, you could also just not run the courier? 🤔

@frederikhors
Copy link
Contributor Author

If you don’t need to send emails from Kratos, you could also just not run the courier? 🤔

Sorry I meant not as fast as 1 second delay.

I'm fine waiting 30 seconds or more.

@aeneasr
Copy link
Member

aeneasr commented Oct 30, 2023

#3601 will make this configurable

@aeneasr aeneasr closed this Oct 30, 2023
@frederikhors frederikhors deleted the patch-1 branch October 30, 2023 11:03
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