router icon indicating copy to clipboard operation
router copied to clipboard

Remove what should be unnecessary wrapping closure from `selection_set_is_fully_local_from_all_nodes()`

Open sachindshinde opened this issue 1 year ago • 4 comments

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 avatar Jun 08 '24 05:06 sachindshinde

@sachindshinde, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

github-actions[bot] avatar Jun 08 '24 05:06 github-actions[bot]

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

router-perf[bot] avatar Jun 08 '24 05:06 router-perf[bot]

@sachindshinde is this still something we need to get in to dev? or was this simply to please the CI?

lrlna avatar Jun 21 '24 12:06 lrlna

Actually, can we not just do this once unconditionally at the start of the function? It doesn't look that hot...

goto-bus-stop avatar Jun 25 '24 08:06 goto-bus-stop

what do we want to do with this PR @goto-bus-stop @sachindshinde? shall we close it entirely?

lrlna avatar Aug 26 '24 09:08 lrlna

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.

goto-bus-stop avatar Aug 26 '24 09:08 goto-bus-stop

Ok, closing this then.

lrlna avatar Sep 04 '24 10:09 lrlna