Feat/supervisor #64
Feat/supervisor #64
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @kooksee, I'm Gemini Code Assist1! 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 introduces a foundational shift in the application's architecture, primarily by integrating a new service supervision framework. This change significantly enhances the reliability and manageability of services by providing a centralized mechanism for starting, stopping, and monitoring them. Alongside this, numerous core components have undergone extensive refactoring to improve modularity, consistency, and error handling, paving the way for future development and easier maintenance. The update also brings a new, integrated scheduler and streamlines the build process, contributing to a more robust and developer-friendly ecosystem.
Highlights
- New Service Supervision Framework: Implemented a robust service supervision framework (
core/supervisor) for managing and monitoring application services, replacing previous ad-hoc server management and enabling centralized control over service lifecycles. - Modular Core Component Refactoring: Significant overhaul of core components including lifecycle management, logging, metrics, PID file handling, signal handling, and tracing. This enhances modularity, consistency, and maintainability across the codebase.
- Integrated Scheduler Component: Introduced a comprehensive scheduler (
core/scheduler) supporting cron, one-time, and periodic jobs, seamlessly integrated with the new service supervision framework for automated task execution. - Build System Transition: Migrated the project's build process from Makefiles and Goyek tasks to a
go-taskbased system, simplifying build automation and improving developer experience. - API Gateway & Client Refactoring: Major refactoring of the API Gateway (
pkg/gateway) and gRPC/REST clients (clients/grpcc,clients/resty), improving their architecture, error handling, and overall reliability. - Version 2.0.0 Release: This pull request marks a major version increment to
v2.0.0, reflecting extensive breaking changes and architectural improvements across the entire project.
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.
Footnotes
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a major refactoring and feature addition, bumping the module version to v2. The core change is the introduction of a supervisor package based on suture for more robust service lifecycle management, which is a significant architectural improvement. The build system is migrated from Makefile to go-task, and the application startup logic is refactored into a lavabuilder. Many packages, including lifecycle, scheduler, clients, and commands, have been updated to align with these changes. The error handling is improved by more consistent use of the result type. Overall, these are excellent changes that improve the structure and robustness of the project. However, I've found a few issues, mainly a recurring error related to header iteration logic that seems to be based on a non-existent method, which could be critical.
| for _, value := range rsp.Header().All() { | ||
| acquireCookie := fasthttp.AcquireCookie() | ||
| if err := acquireCookie.ParseBytes(value); err != nil { | ||
| j.log.Err(err, ctx).Msg("failed to parse cookie") | ||
| } else { | ||
| j.cookies[string(acquireCookie.Key())] = acquireCookie | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
The logic for parsing cookies from the response header is incorrect. The previous implementation used VisitAllCookie, which correctly iterates over Set-Cookie headers. The new implementation attempts to use a non-existent All() method and seems to intend to iterate over all response headers, trying to parse each as a cookie. This will not compile and is logically incorrect. You should only parse Set-Cookie headers. Please revert to using VisitAllCookie.
| for _, value := range rsp.Header().All() { | |
| acquireCookie := fasthttp.AcquireCookie() | |
| if err := acquireCookie.ParseBytes(value); err != nil { | |
| j.log.Err(err, ctx).Msg("failed to parse cookie") | |
| } else { | |
| j.cookies[string(acquireCookie.Key())] = acquireCookie | |
| } | |
| }) | |
| } | |
| rsp.Header().VisitAllCookie(func(key, value []byte) { | |
| acquireCookie := fasthttp.AcquireCookie() | |
| if err := acquireCookie.ParseBytes(value); err != nil { | |
| j.log.Err(err, ctx).Msg("failed to parse cookie") | |
| } else { | |
| j.cookies[string(acquireCookie.Key())] = acquireCookie | |
| } | |
| }) |
| for key, value := range header.All() { | ||
| md.Append(convert.BtoS(key), convert.BtoS(value)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The header variable is a *lava.RequestHeader (an alias for *fasthttp.RequestHeader), which does not have an All() method. This will cause a compilation error. Please use the VisitAll method to iterate over the headers.
| for key, value := range header.All() { | |
| md.Append(convert.BtoS(key), convert.BtoS(value)) | |
| }) | |
| } | |
| header.VisitAll(func(key, value []byte) { | |
| md.Append(convert.BtoS(key), convert.BtoS(value)) | |
| }) |
| for k, v := range ctx.Response.Header.All() { | ||
| w.Header().Add(string(k), string(v)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The fasthttp.ResponseHeader type does not have an All() method. This code will not compile. You should use the VisitAll method to iterate over the headers, as was done in the previous version of the code.
| for k, v := range ctx.Response.Header.All() { | |
| w.Header().Add(string(k), string(v)) | |
| }) | |
| } | |
| ctx.Response.Header.VisitAll(func(k, v []byte) { | |
| w.Header().Add(string(k), string(v)) | |
| }) |
| for key, value := range rpcReq.rspHeader.All() { | ||
| reqMetadata.Set(convert.BtoS(key), convert.BtoS(value)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The rpcReq.rspHeader is a *fasthttp.ResponseHeader, which does not have an All() method. This will cause a compilation error. Please use the VisitAll method to iterate over the headers.
| for key, value := range rpcReq.rspHeader.All() { | |
| reqMetadata.Set(convert.BtoS(key), convert.BtoS(value)) | |
| }) | |
| } | |
| rpcReq.rspHeader.VisitAll(func(key, value []byte) { | |
| reqMetadata.Set(convert.BtoS(key), convert.BtoS(value)) | |
| }) |
| for key, value := range h.All() { | ||
| md.Append(convert.BtoS(key), convert.BtoS(value)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The h variable, which is a *lava.ResponseHeader (an alias for *fasthttp.ResponseHeader), does not have an All() method. This will cause a compilation error. Please use the VisitAll method to iterate over the headers.
| for key, value := range h.All() { | |
| md.Append(convert.BtoS(key), convert.BtoS(value)) | |
| }) | |
| } | |
| h.VisitAll(func(key, value []byte) { | |
| md.Append(convert.BtoS(key), convert.BtoS(value)) | |
| }) |
| opts := cfg.Client.ToOpts() | ||
| opts = append(opts, grpc.WithResolvers(cfg.Resolvers...)) | ||
| opts = append(opts, grpc.WithChainUnaryInterceptor(unaryInterceptor(mm))) | ||
| opts = append(opts, grpc.WithChainStreamInterceptor(streamInterceptor(mm))) |
There was a problem hiding this comment.
The previous implementation of appending to the opts slice in a single append call was more idiomatic and potentially more performant. The current implementation with multiple append calls might lead to unnecessary reallocations. Consider reverting to the single append call.
| opts := cfg.Client.ToOpts() | |
| opts = append(opts, grpc.WithResolvers(cfg.Resolvers...)) | |
| opts = append(opts, grpc.WithChainUnaryInterceptor(unaryInterceptor(mm))) | |
| opts = append(opts, grpc.WithChainStreamInterceptor(streamInterceptor(mm))) | |
| opts := append(cfg.Client.ToOpts(), | |
| grpc.WithResolvers(cfg.Resolvers...), | |
| grpc.WithChainUnaryInterceptor(unaryInterceptor(mm)), | |
| grpc.WithChainStreamInterceptor(streamInterceptor(mm)), | |
| ) |
kooksee
force-pushed
the
feat/supervisor
branch
from
4faded1 to
4826bdd
Compare
kooksee
force-pushed
the
feat/supervisor
branch
from
4826bdd to
5bcfc37
Compare
kooksee
force-pushed
the
feat/supervisor
branch
from
7fec15d to
cc31de1
Compare
kooksee
force-pushed
the
feat/supervisor
branch
from
cc31de1 to
968d6c9
Compare
1 participant
No description provided.