Skip to content
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

MSC3005: Streaming Federation Events #3005

Closed
wants to merge 16 commits into from

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Feb 10, 2021

Rendered

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

Copy link
Contributor Author

@ShadowJonathan ShadowJonathan left a comment

Choose a reason for hiding this comment

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

Wrote some sleep-deprived comments that i know are absolute issues that need to be addressed here, this MSC requires more comments.

proposals/3005-federation-websockets.md Outdated Show resolved Hide resolved
proposals/3005-federation-websockets.md Outdated Show resolved Hide resolved
proposals/3005-federation-websockets.md Outdated Show resolved Hide resolved
proposals/3005-federation-websockets.md Outdated Show resolved Hide resolved
proposals/3005-federation-websockets.md Outdated Show resolved Hide resolved
proposals/3005-federation-websockets.md Outdated Show resolved Hide resolved
proposals/3005-federation-websockets.md Outdated Show resolved Hide resolved
proposals/3005-federation-websockets.md Outdated Show resolved Hide resolved
@ShadowJonathan ShadowJonathan marked this pull request as ready for review February 10, 2021 22:54
@ShadowJonathan ShadowJonathan changed the title [MSCXXXX] Federation over WebSockets [MSC3005] Federation over WebSockets Feb 10, 2021
@uhoreg uhoreg changed the title [MSC3005] Federation over WebSockets MSC3005: Federation over WebSockets Feb 10, 2021
@uhoreg uhoreg added kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal labels Feb 10, 2021
@ShadowJonathan
Copy link
Contributor Author

Small PSA: I might refactor this MSC into a "generic streaming" spec instead, due to some insightful discussion i had.

@ShadowJonathan ShadowJonathan marked this pull request as draft February 11, 2021 08:08
@ShadowJonathan ShadowJonathan changed the title MSC3005: Federation over WebSockets MSC3005: Streaming Federation Events Feb 11, 2021
| :----------: | ---------------------------------- | ------------------------ | ------------------- |
| `fmt` | Chosen format | `integer | string` | (Cannot be omitted) |
| `ext` | Echoed extension settings | `object{string: value}` | `{}` |
| `ext_op_map` | Mapped extension-frames-to-opcodes | `object{string: string}` | `{}` |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can also be ext_ops, though i wasn't sure.

@ShadowJonathan ShadowJonathan marked this pull request as ready for review February 11, 2021 11:42
Comment on lines +273 to +276
Second value; a free `value` detailing the error of processing this EDU. It should probably be a
`string`, or an `object` with an `"error"` key, but this is not absolutely required.

How server implementations react to PDU errors is out of scope for this proposal.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original spec is incredibly lax with how the "processing results" are defined, so I don't have a choice to make it just as ambiguous.

@ShadowJonathan
Copy link
Contributor Author

Concerns from @Half-Shot re: this, that the spec should avoid bloat, this is a new protocol which'd be implemented in the spec, and matrix has so far been wanting to avoid reinventing the wheel, generally being in favour of using something else that's "out there" before using this protocol.

@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Feb 15, 2021

Some extra discussion on #matrix-spec:matrix.org has yielded the following feedback i'll be implementing;

  • Versioning (Still thinking about it, I need to think of a proper way that doesn't inhabit the entire front-end of the stream session)
  • Changing the assumption of the underlying connection to be a framed one.
    • This is done to deduplicate efforts of framing and make it possible to take advantage of underlying framing in many situations.
    • Pure-streaming connection types will have to implement a "shim" of this, e.g. plastering opcode+payload length in front to segment each chunk into a frame, this is up to those individual proposals to implement and specify (if needed)
    • "the underlying connection MUST be framed, either through native capabilities (WebSocket frames), or emulated (TCP with payload length header)"

  • Make the heartbeat opcodes completely optional, and put these into a dedicated extensions (with their extension frames being freely mappable)

@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
@ShadowJonathan
Copy link
Contributor Author

I believe that this MSC does no longer have much going for it at this stage.

While I believe that a binary&streaming-based solution can speed up raw transfers and decrease raw latency of federation traffic, it will increase implementation complexity, and possibly heighten the barrier to entry for a matrix homeserver.

I do ask the SCT to consider ways to speed up raw transfers, but I don't think I have confidence that this MSC, at this time, and possibly with me as the author, can achieve something like that.

Thus, thanks for your help with this, I am abandoning this MSC.

For anyone reading this at any point in the future, I invite them to take note from the proposal contents to make a better solution with a better context or perspective on this issue.

@turt2live turt2live added the abandoned A proposal where the author/shepherd is not responsive label Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned A proposal where the author/shepherd is not responsive kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants