runtime renaming #68

hzxuzhonghu · 2025-08-29T09:34:10Z

No description provided.

gemini-code-assist

Summary of Changes

Hello @hzxuzhonghu, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors and renames several core components within the orion-proxy and orion-xds crates to improve clarity and align naming conventions with their specific purpose, particularly concerning the XDS runtime. The changes streamline the codebase by replacing generic 'service' terminology with more precise 'XDS' references, enhancing readability and maintainability.

Highlights

XDS Runtime Renaming: The primary focus of this pull request is to rename various components related to the XDS (eXtensible Discovery Service) runtime. This includes renaming the ServiceInfo struct to XDSConfiguration, and updating associated variables and function calls (e.g., spawn_services_runtime_from_thread to spawn_xds_runtime_from_thread).
Refactoring of Runtime Spawning: The functions responsible for spawning proxy and XDS runtimes have been refactored. This includes simplifying the return types of spawn_proxy_runtime_from_thread and spawn_xds_runtime_from_thread and adjusting error handling within tokio::select! blocks.
XDS Client Function Renaming: The core asynchronous function spawn_services has been renamed to run_xds_client to better reflect its role in managing the XDS client.
Minor Code Simplifications: Unnecessary ChildTask assignments in tokio::spawn calls have been removed, and variable names within the XDS client have been made more consistent (e.g., outbound_request_stream to request_stream).
Development Environment Update: The .gitignore file has been updated to include .vscode directories, preventing IDE-specific files from being committed.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request primarily focuses on renaming components for clarity, such as ServiceInfo to XDSConfiguration. While many of these changes improve readability, I've identified two significant issues. First, a critical issue in orion-proxy/src/xds_configurator.rs where a background task is no longer properly managed, which could lead to a resource leak. Second, in orion-proxy/src/proxy.rs, an error returned from start_proxy is being ignored, potentially hiding runtime failures. I've provided code suggestions to address both of these problems.

orion-proxy/src/xds_configurator.rs

orion-proxy/src/proxy.rs

hzxuzhonghu · 2025-09-04T08:38:19Z

ping @awgn @dawid-nowak

fciaccia · 2025-08-29T15:22:04Z

orion-proxy/src/proxy.rs


 #[derive(Debug, Clone)]
-struct ServiceInfo {
+struct XDSConfiguration {


Both the runtime and this data type have a more generic naming because they are not used only to startup the xDS runtime, but to startup a runtime that hosts multiple services that are not part of the data path: xDS updates, access logger, admin interface, tracing. We used to have an xDS naming before and changed it to Service now. I believe it would be wrong to rename it back to just XDS{Configuration,Runtime,..}.

fciaccia · 2025-08-29T15:25:44Z

orion-proxy/src/proxy.rs

 }

-fn spawn_services_runtime_from_thread(
+fn spawn_xds_runtime_from_thread(


As per the ServiceInfo structure, I would not rename this free function name.

orion-proxy/src/xds_configurator.rs

awgn · 2025-09-04T09:29:30Z

orion-proxy/src/proxy.rs


 #[derive(Debug, Clone)]
-struct ServiceInfo {
+struct XDSConfiguration {


ServiceInfo includes a set of services to be run in a dedicated Tokio runtime. One of them is the XDS, but there are others tasks like for example access logs, metrics exporter and the admin interface. Not sure XDSConfiguration is the best name...

cc @fciaccia ServiceInfo is not a concrete meaning, as to access logs, metrics exporter and the admin interface, they should have their own runtime

It is debatable whether or not each service should have its own runtime. As of now, we have N runtimes for the data path and a single runtime for all the other control and accessory services. Initially this secondary runtime only had the xDS service running, so it made sense to start adding "services" to that.

The ServiceInfo structure itself exists only to accomodate a clippy warning that was signaling the spawn_services function to have too many arguments.

Just renaming the structure and relative functions is inconsistent in my opinion. If we want to rename the runtime, then we should also split the services into multiple runtimes as you suggest, but not as it is done in this current PR.

Yeah, i realized it

awgn · 2025-09-04T09:32:28Z

ping @awgn @dawid-nowak

wrote a comment on proxy.rs about ServiceInfo. The rest looks good to me.

hzxuzhonghu

Ah, i see recent sync prs add metrics and accesslog. I will revert serviceInfo first, can refactor them later

Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>

hzxuzhonghu · 2025-09-11T03:29:59Z

orion-lib/src/listeners/listeners_manager.rs

-                else => {
-                    warn!("All listener manager channels are closed...exiting");
-                    return Err("All listener manager channels are closed...exiting".into());
+                _ = ct.cancelled() => {


@fciaccia @awgn Fixed the hack sender holder with a cancel token, we should use the token to support graceful shutdown of listener later

kmesh-bot · 2025-09-12T01:24:19Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kmesh-bot requested a review from dawid-nowak August 29, 2025 09:34

gemini-code-assist bot reviewed Aug 29, 2025

View reviewed changes

kmesh-bot added the size/M label Aug 29, 2025

gemini-code-assist bot reviewed Aug 29, 2025

View reviewed changes

orion-proxy/src/xds_configurator.rs Show resolved Hide resolved

orion-proxy/src/proxy.rs Outdated Show resolved Hide resolved

hzxuzhonghu mentioned this pull request Sep 3, 2025

Optional XDS service, bug fixed. #82

Closed

fciaccia reviewed Sep 4, 2025

View reviewed changes

awgn reviewed Sep 4, 2025

View reviewed changes

hzxuzhonghu commented Sep 5, 2025

View reviewed changes

hzxuzhonghu force-pushed the cleanup-launch branch from 001461d to 7c730bf Compare September 9, 2025 06:42

kmesh-bot added size/L and removed size/M labels Sep 9, 2025

hzxuzhonghu added 4 commits September 10, 2025 11:43

Rename to be more clear

06c200c

Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>

update

723ae54

Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>

Added a signal handler to falilitate graceful shutdown later

b124d33

Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>

Fix clippy

35bca98

Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>

hzxuzhonghu force-pushed the cleanup-launch branch from 16d178e to 35bca98 Compare September 10, 2025 03:53

remove the hack sender channel holder

597914a

Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>

hzxuzhonghu commented Sep 11, 2025

View reviewed changes

hzxuzhonghu added lgtm approved labels Sep 12, 2025

kmesh-bot merged commit 59adc36 into kmesh-net:main Sep 12, 2025
4 checks passed

runtime renaming #68

runtime renaming #68

Uh oh!

Conversation

hzxuzhonghu commented Aug 29, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

hzxuzhonghu commented Sep 4, 2025

Uh oh!

fciaccia Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

fciaccia Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

awgn Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

fciaccia Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

awgn commented Sep 4, 2025

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

kmesh-bot commented Sep 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

awgn Sep 4, 2025 •

edited

Loading