Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

push federation retry limiter down to matrixfederationclient #2050

Merged
merged 4 commits into from Mar 23, 2017

Conversation

Projects
None yet
2 participants
Member

richvdh commented Mar 23, 2017

rather than having to instrument everywhere we make a federation call, make
the MatrixFederationHttpClient manage the retry limiter.

This might result in a couple of places getting NotRetryingDestination excepitons where they weren't previously expected - but I suspect that most of those code paths are like the send-invite path and don't have any exception handling for any other exceptions either (#2047), so this isn't making things much worse.

Hopefully this will fix #1737, #1829, #1463 and #1569.

MatrixFederationHttpClient: clean up
rename _create_request to _request, and push ascii-encoding of `destination`
and `path` down into it

@richvdh richvdh assigned erikjohnston and richvdh and unassigned erikjohnston Mar 23, 2017

Member

richvdh commented Mar 23, 2017

balls, the tests are failing

push federation retry limiter down to matrixfederationclient
rather than having to instrument everywhere we make a federation call,
make the MatrixFederationHttpClient manage the retry limiter.

@richvdh richvdh assigned erikjohnston and unassigned richvdh Mar 23, 2017

Owner

erikjohnston commented Mar 23, 2017

+1 on moving this logic down.

I think we should seriously think about the UX of limiting all requests to a domain. E.g. if I can't invite someone or lookup an alias on a domain for several hours after it coming back up its going to be quite frustrating. I suggest we add a param (that defaults to off) that allows us to specify that some requests shouldn't be subject to retry

Member

richvdh commented Mar 23, 2017

shouldn't be subject to retry

s/retry/backoff/, I assume

I certainly wouldn't be averse to such a parameter. Presumably if such a request succeeded, we'd need to reset the backoff so that subsequent requests also didn't get backed off. (If I invite someone, it's no good being able to successfully invite them if I can't then speak to them).

Member

richvdh commented Mar 23, 2017

@erikjohnston: PTAL?

Ignore backoff history for invites, aliases, and roomdirs
Add a param to the federation client which lets us ignore historical backoff
data for federation queries, and set it for a handful of operations.
Owner

erikjohnston commented Mar 23, 2017

lgtm, modulo test errors

@richvdh richvdh merged commit 9397edb into develop Mar 23, 2017

5 of 8 checks passed

Sytest Dendron (Commit) Build #1794 origin/rav/federation_backoff in progress...
Details
Sytest Dendron (Merged PR) Build started sha1 is merged.
Details
Sytest SQLite (Commit) Build #2688 origin/rav/federation_backoff in progress...
Details
Sytest Postgres (Commit) Build #2615 origin/rav/federation_backoff succeeded in 8 min 11 sec
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Merged PR) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@richvdh richvdh deleted the rav/federation_backoff branch Mar 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment