Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Fix improper or missing room_stats updates when handling state events (deltas).#5691

Closed
reivilibre wants to merge 5 commits into
matrix-org:developfrom
reivilibre:rei/room_stats_fix
Closed

Fix improper or missing room_stats updates when handling state events (deltas).#5691
reivilibre wants to merge 5 commits into
matrix-org:developfrom
reivilibre:rei/room_stats_fix

Conversation

@reivilibre
Copy link
Copy Markdown
Contributor

@reivilibre reivilibre commented Jul 16, 2019 •
edited
Loading

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off

@reivilibre reivilibre changed the base branch from master to develop July 16, 2019 12:38
@reivilibre reivilibre changed the title WIP: Fix room stats delta handling. Fix room stats delta handling. Jul 16, 2019
@reivilibre reivilibre changed the title Fix room stats delta handling. Fix improper or missing room_stats updates when handling state events (deltas). Jul 16, 2019
@reivilibre reivilibre marked this pull request as ready for review July 16, 2019 12:46
@reivilibre reivilibre requested a review from hawkowl July 16, 2019 12:55
@reivilibre
Create room_stats rows for new rooms. #5624
20ae4af 
Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>
@reivilibre
Don't decrease left_members if the user is joining for the first time.
181c1a6 
Fixes #5423

Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>
@reivilibre
Update state_events and current_state_events upon receipt of a state
dc68c2a 
event #5690.

Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>
@reivilibre
Changelog for #5691
8502c66 
Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>
@reivilibre reivilibre closed this Jul 17, 2019
@reivilibre reivilibre reopened this Jul 17, 2019
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 17, 2019 •
edited
Loading

Codecov Report

Merging #5691 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #5691      +/-   ##
===========================================
+ Coverage    63.32%   63.35%   +0.02%     
===========================================
  Files          331      331              
  Lines        36140    36144       +4     
  Branches      5954     5956       +2     
===========================================
+ Hits         22887    22900      +13     
+ Misses       11622    11613       -9     
  Partials      1631     1631

@reivilibre
Copy link
Copy Markdown
Contributor Author

reivilibre commented Jul 17, 2019 •
edited
Loading

Hrmm. It has come to my attention that the fix proposed here for #5690 isn't quite correct.

For rooms joined over federation, (e.g. librepush.net joining a room on matrix.org), state_events in librepush.net's room_stats will not count state events that have been overwritten in the past – I guess these don't cause deltas.

Edit: On the other hand, if a user makes the membership state transition leave→join and then you join a room over federation, the left_members stat is still sane and isn't negative – somehow. I was expecting it to go to -1 since the user left a leave state…

Edit: This state_events discrepancy will appear during backfill, as deltas seem to be coalesced.
e.g. if we have server S and T in federation, the following steps:

  • Take T offline
  • On S, send state events (transitions) A→B and B→C from S with the same state key/type. (A, B and C here represent some state content.)
  • Bring T online again and wait for it to backfill.

Lead to T handling the delta A→C and increasing state_events by 1 in its room_stats whereas S has increased its by 2 (the actual № state events).

I'm not (yet) sure how to address this.

@reivilibre reivilibre self-assigned this Jul 18, 2019
@reivilibre
Disuse room_stats.state_events (relates to #5690)
94463bf 
since it is tricky to maintain and has no known use case (for now).

Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>
@reivilibre
Copy link
Copy Markdown
Contributor Author

reivilibre commented Jul 18, 2019

The above issue has now been addressed. I am happy for this to be reviewed. (Maybe will save its merge to be done alongside some other upcoming PRs…?)

@reivilibre reivilibre requested a review from a team July 24, 2019 10:22
erikjohnston
erikjohnston approved these changes Jul 29, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

This seems legit to me, but I'd like a quick check from @hawkowl too

@reivilibre
Copy link
Copy Markdown
Contributor Author

reivilibre commented Jul 30, 2019

N.B. This PR may (or may not – we will have to see) be more or less obsolete given that I am going to make quite a bit of change to the room stats stuff. I'll leave it here for now.

@reivilibre
Copy link
Copy Markdown
Contributor Author

reivilibre commented Aug 13, 2019

Obsolete in the face of #5847

@reivilibre reivilibre closed this Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hawkowl hawkowl Awaiting requested review from hawkowl

1 more reviewer

@erikjohnston erikjohnston erikjohnston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@reivilibre reivilibre

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@reivilibre @erikjohnston @richvdh