Revert enable_full_scan

[kyessenov]

Commit Message: Reverts #29873 and #30794

Multiple concerns about the effect of a full scan on LEAST_REQUEST have been raised.
See previous discussions in #11004 and #11006.

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #30812 was opened by kyessenov.

see: more, trace.

[tonya11en]

/retest

[wbpcode]

So, my previous reverting didn't resolve your problem? Now it should do nothing if you didn't set it explicitly?

[wbpcode]

cc @kyessenov may need to kick the ci?

[kyessenov]

/retest

[jkirschner-hashicorp]

Hi all - I'm an external observer who was interested in enable_full_scan and hoping to use it in Envoy 1.29 when I saw the original PR go through.

Given this reversion PR, what is the thinking about when/if this functionality might be reintroduced?

I looked through the references (#11004, #11006) to try to understand why it is being reverted. It seems like the original PR #29873 introduced a subtle/minor breaking change that was fixed in #30794. Per this comment, are we concerned about it still being a breaking change even after #30794, so we're pulling it for now (since it's too late to introduce breaking changes in the 1.29 release cycle)? Or is the reversion due to some other technical concern that I may have overlooked when reading through the references?

Thanks everyone!

[tonya11en]

It won’t be a problem to introduce a full scan option that defaults to off. The problem here was the behavioral change to the default that was rolled into the same patch.

…

On Fri, Nov 10, 2023, at 9:41 AM, Jared Kirschner wrote: Hi all - I'm an external observer who was interested in `enable_full_scan` and hoping to use it in Envoy 1.29 when I saw the original PR go through. Given this reversion PR, what is the thinking about when/if this functionality might be reintroduced? I looked through the references (#11004 <#11004>, #11006 <#11006>) to try to understand why it is being reverted. It seems like the original PR #29873 <#29873> introduced a subtle/minor breaking change that was fixed in #30794 <#30794>. Per this comment <#30768 (comment)>, are we concerned about it still being a breaking change even after #30794 <#30794>, so we're pulling it for now (since it's too late to introduce breaking changes in the 1.29 release cycle)? Or is the reversion due to some other technical concern that I may have overlooked when reading through the references? Thanks everyone! — Reply to this email directly, view it on GitHub <#30812 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIOZ7UFQIJNVAQQNCVYODTYDZROLAVCNFSM6AAAAAA7FC3XOCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBWGE2TKOJRGM>. You are receiving this because you commented.Message ID: ***@***.***>

[jkirschner-hashicorp]

Ah, got it. I may have misunderstood #30794, which I thought removed the "default to on (if host num < choice count)" behavior.

[tonya11en]

@kyessenov I think you'll need to fix formatting to make it pass CI now.

[KBaichoo]

I think this is waiting on you @kyessenov .

/wait

[barroca]

Hello, I have the PR #31146 that start the selection from a random index for the full scan preventing the choice of the same index when the number of requests is the same. Perhaps we can merge it instead of revert?

[kyessenov]

Merged main. Sorry, I forgot to get this over the finish line.
@barroca I think we should combine the reverted code with the fix in a new PR. It's not a big change. We can review it again.

[barroca]

Merged main. Sorry, I forgot to get this over the finish line. @barroca I think we should combine the reverted code with the fix in a new PR. It's not a big change. We can review it again.

No worries, I believe the PR wasn't approved and need some discussion. Hopefully we can come with an agreement.

[tonya11en]

/retest

[tonya11en]

There are merge conflicts now. Luckily, it's just a release note issue. We should wrap this up soon, since there's a few other changes waiting on it.

[kyessenov]

@wbpcode or @htuch to approve this, so I don't have to keep merging main :)

[wbpcode]

/lgtm

let's revert this first and then I think @tonya11en has some work to land. At last, we will have a tool to evaluate our new lb feature.

[repokitteh-read-only]

please specify a single label can be specified

🐱

Caused by: a #30812 (comment) was created by @wbpcode.

see: more, trace.

[wbpcode]

@kyessenov feel free to merge this after resolved conflict and CI.

[wbpcode]

LGTM, thanks.