Ignoring server values for SummaryConfiguration #9965
Conversation
github-actions
bot
added
area: driver
packages/runtime/container-runtime/src/test/runningSummarizer.spec.ts
Outdated
Show resolved
Hide resolved
|
Why are we doing this? #9808 doesn't have info and the PR title / description doesn't talk about it either. In reply to: 1103209578 In reply to: 1103209578 In reply to: 1103209578 In reply to: 1103209578 |
|
The right issue to link to is #9948 In reply to: 1103211098 In reply to: 1103211098 In reply to: 1103211098 In reply to: 1103211098 |
| // Snapshot if 1000 ops received since last snapshot. | ||
| maxOps: 1000, | ||
| // Snapshot if 100 ops received since last snapshot. | ||
| maxOps: 100, |
There was a problem hiding this comment.
I am curious why you chose 100? This means that we will now summarize 10 times more than before. Isn't that a big change?
Also, how do we measure this in order to tweak it? For example, if this results in very frequent summaries and slowing clients down, how will we know?
There was a problem hiding this comment.
Should this be behind a feature flag so that we can revert this if it causes regression?
There was a problem hiding this comment.
Vlad suggested 100 based on the telemetry data. The idea is that the consumers will still be able to override the values as they can today, via the config.
What Vlad wants is to take it away from the service. BTW 9948 is the one where he recommends the removal of the service dependency to track these numbers.
AFAIK Today they are hardcoded on the backend as well.
There was a problem hiding this comment.
Sounds good. My concern is that this may results in client slowing down. We should have a plan if that happens and maybe updating the container runtime options to provide this info is the answer.
There was a problem hiding this comment.
I'd prefer to split change into multiple PRs, with first one only getting rid of service-provided config by copying existing service config into client code. That way it will be so much easier to track what, when & why we changed over time
There was a problem hiding this comment.
ok. will revert all other changes and simply get rid of the service-provided config. Should I get the config values from office.com for instance? Not sure if they might be different.
packages/drivers/file-driver/src/fileDocumentDeltaConnection.ts
Outdated
Show resolved
Hide resolved
|
Yes, it should be 9948 and there is some motivation for 100 on9808 (Vlads comments). In reply to: 1103209578 |
… easier to track the changes.
⯆ @fluid-example/bundle-size-tests: -625 Bytes
Baseline commit: 14454f9 |
github-actions
bot
added
the
area: dds
| summaryOptions: { initialSummarizerDelayMs: 0 }, | ||
| summaryOptions: { | ||
| summaryConfigOverrides: { | ||
| idleTime: 5000, // Current default idleTime is 15000 which will cause some SharedTree tests to timeout. |
There was a problem hiding this comment.
nit: You could make this smaller still so the tests would run faster :-)
Fix for #9948 - Ignoring the configuration values we get from the service for Summmary Configurations and
Adjusting the default values to match the current values we get from the service.