-
Notifications
You must be signed in to change notification settings - Fork 6
migrated server and ProxyWorkerCollection to live as IHost managed BackgroundServices #60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/openai-streaming
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 7 out of 18 changed files in this pull request and generated 1 comment.
Files not reviewed (11)
- src/SimpleL7Proxy/SimpleL7Proxy.csproj: Language not supported
- src/SimpleL7Proxy/IServer.cs: Evaluated as low risk
- src/SimpleL7Proxy/IBackendService.cs: Evaluated as low risk
- src/SimpleL7Proxy/BackendOptions.cs: Evaluated as low risk
- src/SimpleL7Proxy/BackendHost.cs: Evaluated as low risk
- src/SimpleL7Proxy/Program.cs: Evaluated as low risk
- src/SimpleL7Proxy/Backend/IBackendService.cs: Evaluated as low risk
- src/SimpleL7Proxy/Proxy/ProxyWorker.cs: Evaluated as low risk
- src/SimpleL7Proxy/Proxy/ProxyWorkerCollection.cs: Evaluated as low risk
- src/SimpleL7Proxy/Queue/BlockingPriorityQueue.cs: Evaluated as low risk
- src/SimpleL7Proxy/server.cs: Evaluated as low risk
Comments suppressed due to low confidence (3)
src/SimpleL7Proxy/Backend/BackendHostConfig.cs:13
- [nitpick] The Host property is modified in the constructor, which can be confusing. Consider making it read-only or using a different approach.
public string Host { get; private set; } = string.Empty;
src/SimpleL7Proxy/Backend/BackendHostHealth.cs:22
- [nitpick] The variable name lockObj is ambiguous. It should be renamed to lockObject for clarity.
private readonly Lock lockObj = new();
src/SimpleL7Proxy/Backend/BackendHostHealthCollection.cs:15
- The logger parameter should be explicitly typed as 'ILogger' for clarity.
public BackendHostHealthCollection(IOptions<BackendOptions> options, ILogger<BackendHostHealth> logger)
|
||
public class BackendHostHealthCollection : IBackendHostHealthCollection | ||
{ | ||
public List<BackendHostHealth> Hosts { get; } = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Hosts property is initialized with an empty list using invalid C# syntax. It should be 'new List()'.
public List<BackendHostHealth> Hosts { get; } = []; | |
public List<BackendHostHealth> Hosts { get; } = new List<BackendHostHealth>(); |
Copilot uses AI. Check for mistakes.
This pull request includes significant refactoring and improvements to the
SimpleL7Proxy
project. The changes focus on improving the handling of cancellation tokens, making services injectable, and transitioning some classes to use theIHostedService
interface for better integration with the .NET Generic Host.Transition to IHostedService:
ProxyWorkerCollection.cs
: ConvertedProxyWorkerCollection
to extendBackgroundService
for better integration with the hosting environment.Server.cs
: ConvertedServer
to extendBackgroundService
and added handling for application stopping events. [1] [2] [3] [4]Interface and Class Adjustments:
IBackendService.cs
: Updated theStart
method signature to remove theCancellationToken
parameter.ProxyWorker.cs
: Changed theBackends
dependency toIBackendService
to use the interface instead of the concrete class. [1] [2]IServer
interface as it is no longer needed.Refactoring and Dependency Injection Improvements:
Backends.cs
: IntroducedCancellationTokenSource
andCancellationToken
for better task management and addedIHostApplicationLifetime
to handle application stopping events. [1] [2] [3] [4] [5] [6] [7] [8]Program.cs
: Updated the service registration to useIHostedService
and removed direct handling ofCancellationTokenSource
. [1] [2] [3] [4]These changes collectively improve the maintainability and testability of the codebase by leveraging dependency injection and the .NET Generic Host's capabilities.