-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
Signed-off-by: Nicolas Werner <[email protected]>
Signed-off-by: Nicolas Werner <[email protected]>
Signed-off-by: Nicolas Werner <[email protected]>
Signed-off-by: Nicolas Werner <[email protected]>
Signed-off-by: Nicolas Werner <[email protected]>
Signed-off-by: Nicolas Werner <[email protected]>
I'm going to set the review flag on this even though it has failing tests, since I believe they should be fixed by matrix-org/sytest#1040. |
Seems like some integration tests are failing, that I didn't see, when I ran them locally (probably overlooked them). I'll try to fix that soonish. |
Signed-off-by: Nicolas Werner <[email protected]>
Should be fine now, although the tests would now fail, if you don't exclude the keys from /sync. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! a couple of suggestions here.
Co-authored-by: Richard van der Hoff <[email protected]>
Signed-off-by: Nicolas Werner <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
thanks very much! |
Thank you for the review! <3 |
This reverts commit e9eb354.
heads-up that this is being reverted in #9940 |
This leaves out all optional keys from /sync. This should be fine for all clients tested against conduit already, but it may break some clients, as such we should check, that at least most of them don't break horribly and maybe back out some of the individual changes. (We can probably always leave out groups for example, while the others may cause more issues.) Signed-off-by: Nicolas Werner <[email protected]>
Hi,
It would be nice to be more precise. Element Web, Element Android and Element iOS are three different implementations, so may have different bugs :/ |
@bmarty My bad, I missed to put Web. I only tested Element Web, since I have no Android or iOS device. That wasn't intentional, I just missed it! Sorry! |
This leaves out all optional keys from /sync. This should be fine for all clients tested against conduit already, but it may break some clients, as such we should check, that at least most of them don't break horribly and maybe back out some of the individual changes. (We can probably always leave out groups for example, while the others may cause more issues.)
Clients I have tested:
This also needs an updated sytest, see matrix-org/sytest#1040
In my testing this decreases the sync json response to about half, which saves about 30% of data on every /sync. It also makes debugging a lot easier, since the response json is easier to read. :3
Tell me if the news entry is descriptive enough, since some people may want to be aware of this change, if they are client developers or so. Also, my python skills suck, I'm sure there is a tidier way to do this. Also, no idea how to run against the newer sytest version? Is that automatic, after it is merged?
Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.Signed-off-by: Nicolas Werner [email protected]