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

There are summaries that summarize too many ops, indicating we are not summarizing often enough #10007

Merged
merged 4 commits into from
Jun 3, 2022

Conversation

NicholasCouri
Copy link
Contributor

@NicholasCouri NicholasCouri commented Apr 22, 2022

Fix #9908
Initial attempt to adjust the Summary Configuration options now that we do not rely on the Service to provide them:
a) MaxOps (from 1000 - 100 ops)
b) MaxAckWaitTime (from 10 mins down to 2 mins).

@vladsud - I'm not sure if changing idleTime right now, together with MaxOps is the right thing to do - should we wait for more data ? Currently, idleTime is 15 secs.

@agarwal-navin FYI.

@NicholasCouri NicholasCouri requested a review from a team as a code owner April 22, 2022 13:59
@github-actions github-actions bot added area: runtime Runtime related issues base: main PRs targeted against main branch labels Apr 22, 2022
@NicholasCouri NicholasCouri changed the title Reducing MaxOps so we can summarize faster. There are summaries that summarize too many ops, indicating we are not summarizing often enough Apr 22, 2022
@vladsud
Copy link
Contributor

vladsud commented Apr 22, 2022

We need to put note into breaking.md and tell the world that

  • we are changing settings (and say what is changed and how it will impact summarization)
  • Ask each scenario to re-evaluate what is required for that scenario, and if our defaults are not good, provide their own (and describe a way to do so).

This is important, as most likely these settings will not work well for WB - their summaries are pretty heavy.
We should reach out to WB and Skyler's team to heads up.

Please also consider if we want to do #9990 first. I was looking around and discovered a lot of duplication there that sort made sense when we had it provided by service, but now it's one thing. I'd probably keep ISummaryConfiguration and move all summary related settings into ISummaryConfiguration.

@vladsud
Copy link
Contributor

vladsud commented Apr 22, 2022

b) MaxAckWaitTime (from 10 mins down to 2 mins).

I'd rather open an issue to look into it. I see 1200 SummaryAckWaitTimeout events in telemetry over last 30 days. These are bugs. It's very hard to say without looking deeper into it if these bugs would be mitigated by lower timeout.
Please note this is not super-urgent (based on counts), but worth having an issue not to forget about it.

@vladsud
Copy link
Contributor

vladsud commented Apr 22, 2022

I'm not sure if changing idleTime right now, together with MaxOps is the right thing to do - should we wait for more data ? Currently, idleTime is 15 secs.

I've opened #10008 with my thoughts on this.
Short story - I think idleTime knob should go away, and it should be calculated property based on unsummarized ops and maxOps, where maxIdle reaches zero when we have maxOps unsummarized ops, and reaches high numbers if we have few unsummarized ops.

Copy link
Contributor

@vladsud vladsud left a comment

Choose a reason for hiding this comment

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

Looks good, but we have to put a warning in breaking.md about this change and the fact that each experience should test if it works for them and see if they need to provide their own settings as opposed to relying on defaults.

@github-actions github-actions bot removed the area: runtime Runtime related issues label Jun 3, 2022
@NicholasCouri NicholasCouri requested a review from a team as a code owner June 3, 2022 22:06
@github-actions github-actions bot added area: runtime Runtime related issues breaking change This PR or issue would introduce a breaking change labels Jun 3, 2022
@NicholasCouri NicholasCouri merged commit 028a3e1 into microsoft:main Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: runtime Runtime related issues base: main PRs targeted against main branch breaking change This PR or issue would introduce a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There are summaries that summarize too many ops, indicating we are not summarizing often enough
2 participants