Skip to content

Retry /whois to allow for propagation/persistence delay #1222

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 1, 2022
Merged

Retry /whois to allow for propagation/persistence delay #1222

merged 7 commits into from
Apr 1, 2022

Conversation

reivilibre
Copy link
Contributor

Might solve #1180?

Motivated by matrix-org/synapse#12251, which moves the client IP persistence to a different worker than the one that handles /whois, so there is a propagation delay (when it's on the same worker, the information is enriched with unpersisted IPs).

@reivilibre reivilibre requested a review from a team as a code owner March 25, 2022 10:55
@reivilibre reivilibre mentioned this pull request Mar 25, 2022
Merged
richvdh
richvdh reviewed Mar 25, 2022
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

is there any way for us to tighten this test - for example, let's say that as soon as /whois returns a 200, the body must be correct?

@reivilibre
Copy link
Contributor Author

reivilibre commented Mar 25, 2022 •
edited
Loading

is there any way for us to tighten this test - for example, let's say that as soon as /whois returns a 200, the body must be correct?

There is, but I'm not sure how to go about it:

assert_json_list( $value->{sessions}[0]{connections} );
assert_json_keys( $value->{sessions}[0]{connections}[0], qw( ip last_seen user_agent ) );

If {sessions}[0]{connections}[0] exists, but it doesn't contain the right keys, that would be reason to fail immediately.

How can I escape from retry_until_success elegantly? I see it has the ability to specify a callback that tells it whether something should lead to a retry, but I'm not sure how to piece it together. Do you know of any examples that do this kind of thing off the top of your head, that I could have a look at?

richvdh
richvdh reviewed Mar 29, 2022
View reviewed changes
@reivilibre reivilibre requested a review from richvdh March 29, 2022 16:11
@reivilibre reivilibre mentioned this pull request Mar 30, 2022
Merged
richvdh
richvdh reviewed Mar 30, 2022
View reviewed changes
reivilibre and others added 3 commits March 30, 2022 13:44
@reivilibre @richvdh
Update tests/48admin.pl
f1e5bc6 
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@reivilibre
Use repeat_until_true as it is simpler
533f952
@reivilibre
Remove needless timeout override
4e34d67
@reivilibre reivilibre requested a review from richvdh March 31, 2022 10:50
richvdh
richvdh reviewed Apr 1, 2022
View reviewed changes
@reivilibre @richvdh
Apply suggestions from code review
fa1b248 
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@reivilibre reivilibre requested a review from richvdh April 1, 2022 11:54
richvdh
richvdh approved these changes Apr 1, 2022
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm!

@reivilibre reivilibre merged commit fbc57e7 into develop Apr 1, 2022
@reivilibre reivilibre deleted the rei/retry_whois branch April 1, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richvdh richvdh richvdh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@reivilibre @richvdh