FluidFramework icon indicating copy to clipboard operation
FluidFramework copied to clipboard

Add compression to protocol

Open justus-camp opened this issue 2 years ago • 9 comments

Description

To indicate if the op is compressed and what algorithm is being used to compress it, we need to include the algorithm as part of the protocol. This would allow the backend to decompress the op if it wishes.

Reviewer Guidance

I'm not sure what other things need to happen as a result of this change. Will this property get round-tripped back by our supported backends or does additional work need to occur for that to happen?

justus-camp avatar Aug 15 '22 18:08 justus-camp

@milanro, @dstanesc - Just FYI - Early / experimental draft PR.

@dstanesc - I saw that you were experimenting with e2e latency: image

Looks like LZ4 continues to be the best tradeoff?

DLehenbauer avatar Aug 16 '22 18:08 DLehenbauer

Yes, it looks like for the transfer of the payloads we have in mind today for larger data (chunking considered) and the available network speed lz4 adds the least overhead. If overall size is a problem (eg. want to minimize costs), data at rest (ie. persistence) may benefit from additional compression ( ~ 2x).

2nd level of compression could be regarded as an implementation preference of the FRS infrastructure.

This would allow the backend to decompress the op if it wishes.

To @justus-camp comment, would be very interesting to analyze & understand the use-cases mandating backends to decompress container data.

dstanesc avatar Aug 16 '22 19:08 dstanesc

@GaryWilber - FYI, can you please point place in Deli that we need to change to ensure new property will go through?

vladsud avatar Sep 12 '22 19:09 vladsud

@GaryWilber - FYI, can you please point place in Deli that we need to change to ensure new property will go through?

There's two changes that will need to be made.

  1. Update alfred to pass the property to deli - include the new property in this object
  2. Update deli to include the property in sequenced ops - include the new property in this object

GaryWilber avatar Sep 12 '22 20:09 GaryWilber

Do those changes need to go in a separate PR or can they be included in this one?

justus-camp avatar Sep 12 '22 20:09 justus-camp

I think those changes can be included in this PR, though the protocol def updates won't be available in the server packages until there's a version release/bump. So you would need some as any's

GaryWilber avatar Sep 12 '22 20:09 GaryWilber

One interesting observation: Client can't use compression (newly added property) until it's sure relay service (aka ordering service supports it). There are two ways to ensure we are on correct path:

  1. We wait enough time for changes to relay service propagates to 100% production. This might be fine for ODSP, but FRS will have different / slower timeline, so likely we will need to find a way to enable compression only for ODSP workflows (likely - via making it off by default and letting specific applications to enable it).
  2. We have mechanism already for relay service to convey to client what features it supports - see IConnect.supportedFeatures. But there are multiple problems with using it (some are easier to solve, some are harder to solve):
  • ops are generated and flushed to PendingStateManager even without connection.
  • there is current way to propagate that info to container runtime at the moment.
  • there is probably exists some chance that if even if we connected to a relay service that supports propagating this property, there might be fall over where client connects in the future to relay service that does not.

I think the easiest way to solve this for now is to use two properties simultaneously - compression and metadata for now, and eventually remove code that sets metadata (though we likely would need to leave code that checks / reads it forever).

1 might be preferred, but I do not know how long it takes to get there.

vladsud avatar Sep 14 '22 18:09 vladsud

It's a bit unfortunate, but I think your proposal for using metadata and compression property simultaneously seems like the best solution for now. I'm not too familiar with the second option so I'm a bit more inclined to leave it off by default, let applications enable it, and keep writing the metadata field until we're sure the ordering services will support the new property.

justus-camp avatar Sep 14 '22 18:09 justus-camp

Let's go with that (writing both properties and keeping compression off by default for now). I want to see these changes to relay service happen sooner (in main) and then when we are ready to enable compression feature, re-evaluate if changes propagated fully to prod on ODSP side. If they are, then we may not care about supporting (for reading) metadata property.

vladsud avatar Sep 14 '22 20:09 vladsud

From the previous discussion, I've moved these changes to #12135 and am targeting main instead of next

justus-camp avatar Sep 26 '22 22:09 justus-camp