Skip to content

feat(federation): add query planner config flags to the CLI#5769

Merged
goto-bus-stop merged 4 commits intodevfrom
renee/fed-cli-config
Aug 7, 2024
Merged

feat(federation): add query planner config flags to the CLI#5769
goto-bus-stop merged 4 commits intodevfrom
renee/fed-cli-config

Conversation

@goto-bus-stop
Copy link
Member

@goto-bus-stop goto-bus-stop commented Aug 5, 2024

A better way to tinker with options than editing the source.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2024

@goto-bus-stop, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

@router-perf
Copy link

router-perf bot commented Aug 5, 2024

CI performance tests

  • const - Basic stress test that runs with a constant number of users
  • demand-control-instrumented - A copy of the step test, but with demand control monitoring and metrics enabled
  • demand-control-uninstrumented - A copy of the step test, but with demand control monitoring enabled
  • enhanced-signature - Enhanced signature enabled
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • 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
  • 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
  • events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
  • extended-reference-mode - Extended reference mode enabled
  • large-request - Stress test with a 1 MB request payload
  • no-tracing - Basic stress test, no tracing
  • reload - Reload test over a long period of time at a constant rate of users
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • step-local-metrics - Field stats that are generated from the router rather than FTV1
  • step-with-prometheus - A copy of the step test with the Prometheus metrics exporter enabled
  • step - Basic stress test that steps up the number of users over time
  • xlarge-request - Stress test with 10 MB request payload
  • xxlarge-request - Stress test with 100 MB request payload

duckki
duckki reviewed Aug 5, 2024
View reviewed changes
apollo-federation/cli/src/main.rs Outdated
#[arg(long, default_value_t = false)]
enable_defer: bool,
/// Turn off fragment reuse in subgraph queries.
#[arg(long = "no-reuse-fragments", default_value_t = true, action = clap::ArgAction::SetFalse)]
Copy link
Contributor

@duckki duckki Aug 5, 2024

Choose a reason for hiding this comment

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

Wouldn't it be more consistent with harness to have "reuse-fragments" with false as default?

Copy link
Member Author

@goto-bus-stop goto-bus-stop Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

my goal here is to match the default configuration of the query planner.
I would've preferred to have --reuse-fragments to explicitly turn it on and --no-reuse-fragments to explicitly turn it off, and using the QueryPlannerConfig default if neither is given.

Copy link
Contributor

@duckki duckki Aug 6, 2024

Choose a reason for hiding this comment

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

I had a similar discussion with @lrlna over at performance-harness. I thought the same thing and added --disable-reuse-fragments, but I realized it would be another complexity added, while --reuse-fragments <true/false> option is used everywhere else.

duckki
duckki approved these changes Aug 6, 2024
View reviewed changes
Copy link
Contributor

@duckki duckki left a comment

Choose a reason for hiding this comment

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

Besides some bikeshedding on the command line option, it looks good otherwise. We can always change the internal cli tool's interface, if we want to.

apollo-federation/cli/src/main.rs Outdated
#[arg(long, default_value_t = false)]
enable_defer: bool,
/// Turn off fragment reuse in subgraph queries.
#[arg(long = "no-reuse-fragments", default_value_t = true, action = clap::ArgAction::SetFalse)]
Copy link
Contributor

@duckki duckki Aug 6, 2024

Choose a reason for hiding this comment

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

I had a similar discussion with @lrlna over at performance-harness. I thought the same thing and added --disable-reuse-fragments, but I realized it would be another complexity added, while --reuse-fragments <true/false> option is used everywhere else.

@goto-bus-stop goto-bus-stop enabled auto-merge (squash) August 7, 2024 13:35
@goto-bus-stop goto-bus-stop merged commit d2a4b1c into dev Aug 7, 2024
@goto-bus-stop goto-bus-stop deleted the renee/fed-cli-config branch August 7, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@duckki duckki duckki approved these changes

@dariuszkuc dariuszkuc Awaiting requested review from dariuszkuc

@sachindshinde sachindshinde Awaiting requested review from sachindshinde

@SimonSapin SimonSapin Awaiting requested review from SimonSapin

@lrlna lrlna Awaiting requested review from lrlna

@TylerBloom TylerBloom Awaiting requested review from TylerBloom

Assignees

@goto-bus-stop goto-bus-stop

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@goto-bus-stop @duckki