Naming consistency for API/event surface related to Container's connection to delta stream

[markfields]

Two goals:

1. Consistency of naming across all related events and APIs, both public and private
2. Consensus that the names are intuitive as to their meaning and semantics

Here are the proposed new API shapes. See the diff here to see what's changing from the current state.

• IContainer (public):
  • connectionState: ConnectionState
    • Disconnected: no socket connection to delta server
    • Pending: socket connection established but we're not caught up
    • Connected: connected and caught up
  • connect()
    • Calls DeltaManager.connect (and picks up some semantics from old setAutoReconnect?)
  • disconnect()
    • Disconnect from delta stream, until connect() is called again.
  • events emitted:
    • connecting: when starting to establish connection. Some kind of progress tracker passed for observability of estabilishing healthy connection to delta stream
    • connectionStateChanged: when connectionState value changes. arg passed of new ConnectionState.
• IDeltaManager (public):
  • active: boolean
    • Indicates Container.connectionState === Connected and mode is write
  • No connection-related events emitted publicly
• DeltaManager class (internal)
  • connectToDeltaStream
    • Connect to delta stream
  • events emitted (via IDeltaManagerInternalEvents)
    • deltaStreamConnected: when socket is established. Triggers Container transition to Pending state
    • deltaStreamDisconnected: when socket is disconnected. Triggers Container transition to Disconnected state

[jatgarg]

Are we trying to reaplace "Connecting" with "Pending" state?
Also in the events emitted, what is diff between "connecting" and "connectionPending"?

[anthony-murphy]

i've always disliked our read vs write connection at this layer, especially the active concept. the delta manager has separate queues for inbound and outbound, i'd love to unify on those concepts, or downstream and upstream. inbound/downstream would be like read today, and outbound/upstream would be like write today. I don't have a clear picture on the level of control we want to provide here for upstream vs downstream, but even if we don't provide control for them both, we could expose them separately.

[vladsud]

Thanks for putting it together!

Some random thoughts:

• Might be useful to have a single event "connectionStateChange" (with payload having sub-event name) or a single object that raises only connected events, such that their names could match exactly entries in connectionState enum. I think it will be easier for consumers to connect the dots if names match.
• I'd try not to expose connected() getter as much as possible. Consumers need to be aware how it's calculated, or (if they do not) they are likely to run into incorrect interpretation and bugs. I think it's fine to have that property at runtime layer only, where stats are truly boolean (we do not expose "connecting" state there).
• "active" could be renamed (or documented) to mean "client is in the quorum". But I do not think it abstracts from need to know about "read" vs. "write" connections (as obvious next question - when clients could and could not be in quorum).
• I'd prefer not to see connected events on IDeltaManagerEvents at all. They need to move to IDeltaManagerInternalEvents and to be there only for Container class (i.e. consumption within same layer). Container class controls projection of these events to both runtime (in the form of setConnectionState() method) & hosts (events you mention). Plus, events should rarely used across compat boundaries - it's a pain to control and maintain compatibility.

[markfields]

Thanks for the feedback/questions!

Are we trying to reaplace "Connecting" with "Pending" state?

Yes

Also in the events emitted, what is diff between "connecting" and "connectionPending"?

connecting would be when we start trying to connect. This is useful for telemetry on connection health that Loops org wants to have. connectionPending would be when we have a healthy-looking web socket, but aren't caught up yet.

i've always disliked our read vs write connection at this layer, especially the active concept

Good topic, but out of scope for the moment (but maybe I will think about it and include in this proposal). active getter is unchanged here, I was just cataloging everything in the API surface that's exposing anything about connection state.

I'd try not to expose connected() getter as much as possible.

Looks like @scottn12 is removing this in #9439, so I'll update this doc too.

Might be useful to have a single event "connectionStateChange"

I think I like this idea, thanks.

I'd prefer not to see connected events on IDeltaManagerEvents at all.

Good point, yeah let's move them to IDeltaManagerInternalEvents

[markfields]

Updated the original comment and the PR showing the diff.

[markfields]

As discussed in the diff PR, will probably merge connecting event into connectionStateChanged event. See #9098 for an ill-fated attempt at an event like this, btw. And I also would like to be able to pass a progress tracking object to that event so hosts can keep track (for logging) of progress through the constituent steps of connecting. I've been working on and off for 6 months to help Office Loops team understand causes behind slow/inability to connect and this info would be invaluable.

Thinking probably just an object that we silently update, so if too much time elapses they can query it for progress - as opposed to an object that emits fine-grained events along the way. That would be too difficult to maintain and just kind of noisy. The object could have a pretty generic contract full of string-named steps and their status.

[markfields]

Clearing May milestone. This design is living in on this internal Word doc. I will update this issue once that design is finalized.

[ghost]

This issue has been automatically marked as stale because it has had no activity for 180 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!