Rationalize ISummaryConfiguration & ISummaryRuntimeOptions

[vladsud]

Once PR #9965 is in, we should look closer to ISummaryRuntimeOptions:

1. Remove generateSummaries
2. Coalesce disableSummaries & summaryConfigOverrides.disableSummaries
3. Potentially inline summaryConfigOverrides & get rid of ISummaryConfiguration interface
   • Eventually remove ISummaryConfiguration from protocol
   • Or potentially do the opposite - move all summary related settings into ISummaryConfiguration, making ISummaryRuntimeOptions cleaner. I actually like that better :)

[NicholasCouri]

I suppose 1 and 2 can be easily done on main.

@vladsud in order to change ISummaryConfiguration and/or ISummaryRuntimeOptions we will need to change the protocol-definitions and probably do the change on next.
Also, it seems that ISummaryConfiguration is sent to client when they connect via IClientConfiguration. I'm not sure if we should change it now - not seeing many gains, to be honest.

Should I proceed?

If we decide to do it, I would probably move these fields from ISummaryRuntimeOptions to ISummaryConfiguration:

disableSummaries?: boolean;
maxOpsSinceLastSummary?: number;

And ISummaryConfiguration, ISummaryRuntimeOptions would be:

export interface ISummaryConfiguration {
    idleTime: number;
    maxTime: number;
    maxOps: number;
    maxAckWaitTime: number;

    disableSummaries?: boolean;
    maxOpsSinceLastSummary?: number;
}
export interface ISummaryRuntimeOptions {
    /** Override summary configurations set by the server. */
    summaryConfigOverrides?: Partial<ISummaryConfiguration>;

    // Flag that disables putting channels in isolated subtrees for each data store
    // and the root node when generating a summary if set to true.
    // Defaults to FALSE (enabled) for now.
    disableIsolatedChannels?: boolean;

    /**
     * Flag that will enable changing elected summarizer client after maxOpsSinceLastSummary.
     * This defaults to false (disabled) and must be explicitly set to true to enable.
     */
    summarizerClientElection?: boolean;

     /** Options that control the running summarizer behavior. */
     summarizerOptions?: Readonly<Partial<ISummarizerOptions>>;  // which only has disableHeuristics
}

[vladsud]

Once your PR #9965 hits 100% saturation, we will remove ISummaryConfiguration from protocol altogether.

We do not need to wait for it, we can define new version of ISummaryConfiguration in container-runtime layer and start using it, i.e. place it next to ISummaryRuntimeOptions.

I think we should simplify further. I do not like partiality of settings - this will never work (as they work in tandem). So, we either overwrite all settings, or none of them. So summaryConfigOverrides should be optional, but not partial. If it's provided, it overwrites defaults, if it's not provided - it does not overwrite defaults.

I'd suggest removing ISummarizerOptions, and figure out how to incorporate disableHeuristics into summaryConfigOverrides. In other places we do something like that:

interface ISummaryConfigurationCore {
    idleTime: number;
    maxTime: number;
    maxOps: number;
    maxAckWaitTime: number;
    maxOpsSinceLastSummary?: number;
};

type ISummaryConfiguration= 
{
    state: "disabled";
    // We should check, but my cursory glance suggests there is no way to run a summary in this mode.
} | {
    state: "disableHeuristics";
    maxAckWaitTime: number;
    // whatever other settings are used in this mode - I assume maxAckWaitTime is used.
} | ({ state: "enabled";} & ISummaryConfigurationCore);

In other words - it should not be possible to pass (or require) data that are not used.
If it's disabled - then it'd disabled, and it's the end of it, no more needs to be said about any of the properties.

need better names for interfaces - "Core" is just an example of how to slice it.

[vladsud]

Please note that's a breaking change, so it likely needs to go to next branch (or whatever it's called).

[NicholasCouri]

@vladsud we also have the IClientConfiguration that contains the summary get from the server.

Should I remove it as well? Perhaps, we can do it in 2 steps and keep it for now (and use the original one under ISummaryConfigurationOld). Thoughts ?

export interface IClientConfiguration {
    // Max message size the server will accept before requiring chunking
    maxMessageSize: number;

    // Server defined ideal block size for storing snapshots
    blockSize: number;

    // Summary algorithm configuration. This is sent to clients when they connect
    summary: ISummaryConfiguration;
}

[vladsud]

we also have the IClientConfiguration that contains the summary get from the server.

I'd recommend opening a bug and do it in a month.
Roughly in that time frame we should ping Gary & FRS to remove it from service.

[NicholasCouri]

we also have the IClientConfiguration that contains the summary get from the server.

I'd recommend opening a bug and do it in a month. Roughly in that time frame we should ping Gary & FRS to remove it from service.

filed #10050 to track the removal.

[vladsud]

@NicholasCouri, Can you please summarize what's left here?
You have submitted bit PR in this space, so it would be useful to list remaining work to assess its complexity and need.

[NicholasCouri]

@vladsud I think this work is done. Only remaining issue is #10050.