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

only try to LL members after a non-gappy incr sync if the timeline has events #3936

Merged
merged 3 commits into from Sep 24, 2018

Conversation

Projects
None yet
2 participants
@ara4n
Member

ara4n commented Sep 24, 2018

This was okay until we started to try to LL ourselves, which would then cause
us to try to add LL members even if the timeline was empty. This caused an
out of bounds exception

only try to LL members after a non-gappy incr sync if the timeline ha…
…s events.

This was okay until we started to try to LL ourselves, which would then cause
us to try to add LL members even if the timeline was empty.  This caused an
out of bounds exception

@ara4n ara4n requested a review from richvdh Sep 24, 2018

@ara4n

This comment has been minimized.

Member

ara4n commented Sep 24, 2018

(fixes regression in #3916)

@richvdh

well, the code looks fine I guess, so if we're really in a hurry we could merge it. However:

  • I don't really understand why #3916 was a thing: why would we want to include our member event for each and every incremental sync? (vector-im/riot-web#7282 talks about doing this for initial sync, but doesn't explain why it was necessary for incremental sync).
  • given we seem to have sytests which test #3916, could they not be tweaked to catch this edge-case?
@ara4n

This comment has been minimized.

Member

ara4n commented Sep 24, 2018

We LL ourselves in case it's a gappy sync, and in case we were doing LL over gappy syncs (i.e. if #3840 hadn't disabled it). In practice the members will get removed by the anti-redundancy code. In practice I guess we can move it to only apply to initial syncs for now to avoid needless work; will do so.

Yup, the sytests could catch this edge case.

@ara4n

This comment has been minimized.

Member

ara4n commented Sep 24, 2018

(tests got added in matrix-org/sytest#496)

@richvdh

lgtm

@ara4n ara4n merged commit 787d22e into develop Sep 24, 2018

10 checks passed

ci/circleci: sytestpy2 Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgres Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3 Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgres Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ara4n ara4n deleted the matthew/fix-self-ll branch Sep 24, 2018

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