/synapse

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

Remove event fetching from DB threads #835

Merged
merged 1 commit into from Jun 3, 2016

Conversation

Reviewers
No reviews
Assignees

@NegativeMjark NegativeMjark

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@erikjohnston @NegativeMjark @ara4n
@erikjohnston
Owner

erikjohnston commented Jun 3, 2016

No description provided.

@erikjohnston
Remove event fetching from DB threads

All checks have passed

8 successful checks
@matrixbot
Flake8 + Packaging (Commit) — Build #899 origin/erikj/get_event_txn succeeded in 33 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Commit) — Build #859 origin/erikj/get_event_txn succeeded in 6 min 11 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Commit) — Build #871 origin/erikj/get_event_txn succeeded in 5 min 0 sec
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Commit) — Build #939 origin/erikj/get_event_txn succeeded in 1 min 13 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
05e01f2

@erikjohnston erikjohnston assigned NegativeMjark Jun 3, 2016

@NegativeMjark
Contributor

NegativeMjark commented Jun 3, 2016

LGTM

@erikjohnston erikjohnston merged commit f6be734 into develop Jun 3, 2016
8 checks passed

8 checks passed

Flake8 + Packaging (Commit) Build #899 origin/erikj/get_event_txn succeeded in 33 sec
Details
Flake8 + Packaging (Merged PR) Build finished.
Details
Sytest Postgres (Commit) Build #859 origin/erikj/get_event_txn succeeded in 6 min 11 sec
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Commit) Build #871 origin/erikj/get_event_txn succeeded in 5 min 0 sec
Details
Sytest SQLite (Merged PR) Build finished.
Details
Unit Tests (Commit) Build #939 origin/erikj/get_event_txn succeeded in 1 min 13 sec
Details
Unit Tests (Merged PR) Build finished.
Details
@ara4n
Owner

ara4n commented Jun 3, 2016

what's the rationale here?
@erikjohnston
Owner

erikjohnston commented Jun 3, 2016

@ara4n:

what's the rationale here?

Mainly consistency:

  1. Consistency with everywhere else ─ most of these are uncommon code paths.
  2. It means we can get rid of the horrible duplication between the txn and non-txn versions of fetching events (turns out its not easy to split out the common code)
  3. Everything now goes through the same bulk fetching of events code, which is nice.
  4. The event cache no longer needs to be thread safe.
  5. The DB metrics report how long the actual queries take, rather than also including the time it takes to get events.
  6. Fetching events takes a bit of CPU time, and I'd rather do CPU work on the main thread, as otherwise it takes up a DB thread (which are in relatively limited supply)

@richvdh richvdh deleted the erikj/get_event_txn branch Dec 1, 2016

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