Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Handle new command line arguments with "functions-" prefix #1897

Merged
merged 11 commits into from
Sep 15, 2023

Conversation

kshyju
Copy link
Member

@kshyju kshyju commented Sep 11, 2023

Part of #1900
Resolves #1547

This PR adds the relevant changes to read the new command line arguments we plan to send from the host. See the host issue Azure/azure-functions-host#9504 for understanding the need for this change.

Example of command line argument string host will send to worker process.

-host localhost --port 80 --workerId testWorkerId --requestId testId --grpcMaxMessageLength 2147483647 --functions-uri http://localhost:80 --functions-workerid testWorkerId --functions-requestid testId --functions-grpcmaxmessagelength 2147483647

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

@kshyju kshyju marked this pull request as ready for review September 11, 2023 22:05
@kshyju
Copy link
Member Author

kshyju commented Sep 11, 2023

Relevant host change (still in draft because waiting for some lang workers to fix their packages to handle this): https://github.com/Azure/azure-functions-host/pull/9514/files

Copy link
Contributor

@jviau jviau left a comment

Choose a reason for hiding this comment

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

While we are enhancing this, couple comments:

  1. Technically command-line semantics is to start with --, then kebab-case the argument. So it would be --functions-request-id and not --functions-requestid.
  2. Should we map these into a specific configuration section so they don't get accidentally overriden by something unrelated? Maybe everything under Functions:Worker section? (Functions:Worker:HostUri, Functions:Worker:RequestId, Functions:Worker:WorkerId)

@kshyju
Copy link
Member Author

kshyju commented Sep 12, 2023

@jviau Pushed an update to switch to kebab case.

 - Removed Host and Port property and using the new Uri property on GrpcWorkerStartupOptions type.
 - Using switchMap for commandline configuraton provider.
 - Mapped the command line args to "Functions:Worker" section in configuration.
 - Added project reference (Worker.Extensions.Rpc) to Worker.Grpc to reuse the extension method.
@kshyju kshyju requested review from fabiocav and jviau September 14, 2023 15:28
@kshyju
Copy link
Member Author

kshyju commented Sep 14, 2023

@fabiocav @jviau This is ready for another round of review.

Copy link
Contributor

@jviau jviau left a comment

Choose a reason for hiding this comment

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

Looks great! Only concern is the reference to Extensions.Rpc. I suggest we just duplicate code for now. We can follow up later and see if there is an opportunity to make Extensions.Rpc appropriate for DotNetWorker.Grpc to leverage.

…"GetFunctionsHostGrpcUri" in DotnetWorker.Grpc
@kshyju kshyju closed this Sep 15, 2023
@kshyju kshyju reopened this Sep 15, 2023
@kshyju kshyju merged commit f406711 into main Sep 15, 2023
@kshyju kshyju deleted the shkr/gh-1547_new_cmdline_args branch September 15, 2023 23:02
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix command line arguments for worker getting overridden
3 participants