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

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

Merged
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).

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?

tests/48admin.pl Outdated Show resolved Hide resolved
@reivilibre
Copy link
Contributor Author

reivilibre commented Mar 25, 2022

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?

tests/48admin.pl Outdated Show resolved Hide resolved
tests/48admin.pl Outdated Show resolved Hide resolved
tests/48admin.pl Outdated Show resolved Hide resolved
tests/48admin.pl Outdated Show resolved Hide resolved
reivilibre and others added 3 commits March 30, 2022 13:44
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@reivilibre reivilibre requested a review from richvdh March 31, 2022 10:50
tests/48admin.pl Outdated Show resolved Hide resolved
tests/48admin.pl Show resolved Hide resolved
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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants