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

Update sync method to leave out unneeded bytes #8336

Closed

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Sep 17, 2020

(And fix some tests to stop assuming unilateral presence of sync keys)

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

(Partially) addresses #6579 (room.join is not optimized)

Note: This PR is potentially breaking, it still adheres to the spec, but some clients or libraries could have (up to this point) fully assumed that all keys are present in the /sync object.

Signed-off-by: Jonathan de Jong <jonathan@automatia.nl>

@ShadowJonathan
Copy link
Contributor Author

Looks like the tests are blocked because sytest assumes some keys in /sync will be present...

@clokep
Copy link
Member

clokep commented Dec 1, 2020

@ShadowJonathan Curious if you plan to keep working on this? A PR could be made to modify sytest to avoid the unnecessary keys.

@clokep clokep added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Dec 1, 2020
@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Dec 1, 2020

@clokep this is still blocked because of test logic in sytest, which asserts that these keys are needed.

Does synapse still test with sytest?

@clokep
Copy link
Member

clokep commented Dec 2, 2020

@clokep this is still blocked because of test logic in sytest, which asserts that these keys are needed.

Right -- but we can change sytest. sytest isn't immutable.

Does synapse still test with sytest?

Yes.

@ShadowJonathan
Copy link
Contributor Author

Well, then i cant progress on this.

And by the way, if that's so, then sytest is not compatible with the spec, then its incorrect to assert those keys should be there.

The spec says those keys are optional, sytest asserts otherwise, sytest is wrong, and thus should be changed, and thats why its blocked on that.

Further discussion in here, please; matrix-org/sytest#957

And no, this PR is just blocked, if you wanna unblock it, resolve that issue first, please, dont just close it because its taking space.

@ShadowJonathan ShadowJonathan marked this pull request as draft March 9, 2021 11:50
@ShadowJonathan
Copy link
Contributor Author

(Converting to draft due to unknown status of the dependant issue at this time)

@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Apr 10, 2021

Due to no progress with this PR for a while, and because of some outstanding other projects i'd need to focus on, im closing this PR for the time being. I'll probably re-open it if or when i get back on track with misc issues like these.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged z-blocked (Deprecated Label)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants