router
router copied to clipboard
Remove what should be unnecessary wrapping closure from `selection_set_is_fully_local_from_all_nodes()`
This PR is a followup to https://github.com/apollographql/router/pull/5378 . In that PR, removing this wrapping closure triggered 46 router tests to fail specifically on macOS with the message 'GraphQL endpoint exposed' not detected in logs (and this appeared to be consistent across 10+ re-runs, even with different commits due to rebasing on latest dev). It appears the issue is still present in this PR, so this PR acts as a repro.
@sachindshinde, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.
CI performance tests
- [x] step - Basic stress test that steps up the number of users over time
- [ ] reload - Reload test over a long period of time at a constant rate of users
- [ ] step-with-prometheus - A copy of the step test with the Prometheus metrics exporter enabled
- [ ] events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
- [ ] events - Stress test for events with a lot of users and deduplication ENABLED
- [ ] events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
- [ ] events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
- [ ] xlarge-request - Stress test with 10 MB request payload
- [x] const - Basic stress test that runs with a constant number of users
- [ ] events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
- [ ] step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
- [ ] xxlarge-request - Stress test with 100 MB request payload
- [ ] demand-control-uninstrumented - A copy of the step test, but with demand control monitoring enabled
- [ ] no-graphos - Basic stress test, no GraphOS.
- [ ] events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
- [ ] large-request - Stress test with a 1 MB request payload
- [ ] demand-control-instrumented - A copy of the step test, but with demand control monitoring and metrics enabled
@sachindshinde is this still something we need to get in to dev? or was this simply to please the CI?
Actually, can we not just do this once unconditionally at the start of the function? It doesn't look that hot...
what do we want to do with this PR @goto-bus-stop @sachindshinde? shall we close it entirely?
I would close it. The wrapping closure will be inlined by the compiler, so there is no performance impact, and I think it does benefit the readability a bit. I'm not against the change if @sachindshinde prefers it.
Ok, closing this then.