Refactor index manager service #2135
Conversation
|
[CHATOPS:HELP] ChatOps commands.
|
github-actions
bot
added
team/set
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
pkg/manager/index/service/indexer.go
Outdated
| @@ -162,12 +168,12 @@ func (idx *index) Start(ctx context.Context) (<-chan error, error) { | |||
| select { | |||
| case <-ctx.Done(): | |||
| return | |||
| case addr := <-idx.saveIndexTargetAddrCh: | |||
| idx.schMap.Delete(addr) | |||
| case addr := <-idx.saveIndexTargetAddrCh: // this channel value send from execute func after thier CreateIndex operation when argument immediateSaving=false. | |||
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶thier is a misspelling of their (misspell)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2135 +/- ##
==========================================
- Coverage 18.92% 18.06% -0.87%
==========================================
Files 565 563 -2
Lines 65223 64192 -1031
==========================================
- Hits 12346 11597 -749
+ Misses 52081 51931 -150
+ Partials 796 664 -132 ☔ View full report in Codecov by Sentry. |
kpango
force-pushed
the
refactor/manager-index/small-refactor
branch
from
2c0a458
to
3591a6c
Compare
Deploying vald with
|
| Latest commit: |
8f35f90
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a827b526.vald.pages.dev |
| Branch Preview URL: | https://refactor-manager-index-small.vald.pages.dev |
apis/docs/v1/docs.md
Outdated
| | Method Name | Request Type | Response Type | Description | | ||
| | ----------- | -------------------------------------- | -------------------------------------------------------------- | --------------------------------------------------------------- | | ||
| | IndexInfo | [.payload.v1.Empty](#payload-v1-Empty) | [.payload.v1.Info.Index.Count](#payload-v1-Info-Index-Count) | Represent the RPC to get the index information. | | ||
| | IndexDetail | [.payload.v1.Empty](#payload-v1-Empty) | [.payload.v1.Info.Index.Detail](#payload-v1-Info-Index-Detail) | Represent the RPC to get the index information for each agents. | |
There was a problem hiding this comment.
[LanguageTool] reported by reviewdog 🐶
The noun should probably be in the singular form. (EVERY_EACH_SINGULAR[1])
Suggestions: agent
URL: https://languagetool.org/insights/post/word-choice-each-and-every/
Rule: https://community.languagetool.org/rule/show/EVERY_EACH_SINGULAR?lang=en-US&subId=1
Category: GRAMMAR
apis/docs/v1/docs.md
Outdated
|
|
||
| | Field | Type | Label | Description | | ||
| | ------ | -------------------------------------------------------------------------- | -------- | --------------------------- | | ||
| | counts | [Info.Index.Detail.CountsEntry](#payload-v1-Info-Index-Detail-CountsEntry) | repeated | count infos for each agents | |
There was a problem hiding this comment.
[LanguageTool] reported by reviewdog 🐶
The noun should probably be in the singular form. (EVERY_EACH_SINGULAR[1])
Suggestions: agent
URL: https://languagetool.org/insights/post/word-choice-each-and-every/
Rule: https://community.languagetool.org/rule/show/EVERY_EACH_SINGULAR?lang=en-US&subId=1
Category: GRAMMAR
| @@ -53,3 +53,13 @@ func (s *server) IndexInfo(ctx context.Context, _ *payload.Empty) (res *payload. | |||
| Indexing: s.indexer.IsIndexing(), | |||
| }, nil | |||
| } | |||
|
|
|||
| func (s *server) IndexDetail(ctx context.Context, _ *payload.Empty) (res *payload.Info_Index_Detail, err error) { | |||
| ctx, span := trace.StartSpan(ctx, "vald/manager-index.IndexDetail") | |||
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
ineffectual assignment to ctx (ineffassign)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -53,3 +53,13 @@ | |||
| Indexing: s.indexer.IsIndexing(), | |||
| }, nil | |||
| } | |||
|
|
|||
| func (s *server) IndexDetail(ctx context.Context, _ *payload.Empty) (res *payload.Info_Index_Detail, err error) { | |||
| ctx, span := trace.StartSpan(ctx, "vald/manager-index.IndexDetail") | |||
Check warning
Code scanning / CodeQL
Useless assignment to local variable
kpango
force-pushed
the
refactor/manager-index/small-refactor
branch
from
3591a6c
to
412e28e
Compare
kpango
force-pushed
the
refactor/manager-index/small-refactor
branch
from
412e28e
to
f1760bb
Compare
kpango
force-pushed
the
refactor/manager-index/small-refactor
branch
2 times, most recently
from
bf7c505
to
8aeb635
Compare
kpango
force-pushed
the
refactor/manager-index/small-refactor
branch
from
30ec1b5
to
44994b1
Compare
kpango
force-pushed
the
refactor/manager-index/small-refactor
branch
from
44994b1
to
189ec53
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Out of diff range and nitpick comments (3)
pkg/manager/index/service/indexer.go (2)
46-47: Add documentation for new methodLoadIndexDetail.Consider adding a comment to document the purpose and usage of the new
LoadIndexDetailmethod in theIndexerinterface.
69-70: Remove redundantemptyvariable.The
emptyvariable is defined but not used anywhere in the code. Consider removing it to clean up the code.pkg/agent/core/ngt/service/ngt_test.go (1)
Line range hint
45-45: Remove unused imports.The import statements for
kvs,metadata, andvqueueshould be verified for usage within the file. Unused imports should be removed to keep the code clean and maintainable.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (10)
apis/grpc/v1/manager/index/index_manager.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/manager/index/index_manager_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.json.gois excluded by!**/*.pb.json.goapis/grpc/v1/payload/payload_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goexample/client/go.modis excluded by!**/*.modexample/client/go.sumis excluded by!**/*.sumgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sumrust/Cargo.lockis excluded by!**/*.lock
Files selected for processing (38)
- .github/ISSUE_TEMPLATE/bug_report.md (1 hunks)
- .github/ISSUE_TEMPLATE/security_issue_report.md (1 hunks)
- .github/PULL_REQUEST_TEMPLATE.md (1 hunks)
- .github/workflows/release.yml (1 hunks)
- Makefile.d/dependencies.mk (1 hunks)
- Makefile.d/functions.mk (1 hunks)
- apis/docs/v1/docs.md (3 hunks)
- apis/proto/v1/manager/index/index_manager.proto (1 hunks)
- apis/proto/v1/payload/payload.proto (1 hunks)
- apis/swagger/v1/manager/index/index_manager.swagger.json (2 hunks)
- internal/info/info.go (5 hunks)
- internal/net/grpc/errdetails/errdetails.go (2 hunks)
- internal/observability/trace/status.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/insert.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/remove.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/search.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/update.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/flush.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/insert.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/linear_search.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/remove.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/search.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/update.go (1 hunks)
- pkg/agent/core/ngt/service/ngt_test.go (5 hunks)
- pkg/manager/index/handler/grpc/handler.go (1 hunks)
- pkg/manager/index/handler/grpc/handler_test.go (1 hunks)
- pkg/manager/index/service/indexer.go (8 hunks)
- pkg/manager/index/service/indexer_test.go (1 hunks)
- pkg/manager/index/service/option.go (4 hunks)
- pkg/manager/index/usecase/indexer.go (1 hunks)
- rust/libs/proto/src/manager.index.v1.tonic.rs (5 hunks)
- rust/libs/proto/src/payload.v1.rs (1 hunks)
- versions/GOLANGCILINT_VERSION (1 hunks)
- versions/K3S_VERSION (1 hunks)
- versions/NGT_VERSION (1 hunks)
- versions/PROMETHEUS_STACK_VERSION (1 hunks)
- versions/YQ_VERSION (1 hunks)
- versions/actions/SOFTPROPS_ACTION_GH_RELEASE (1 hunks)
Files skipped from review due to trivial changes (1)
- versions/actions/SOFTPROPS_ACTION_GH_RELEASE
Files skipped from review as they are similar to previous changes (33)
- .github/ISSUE_TEMPLATE/bug_report.md
- .github/ISSUE_TEMPLATE/security_issue_report.md
- .github/PULL_REQUEST_TEMPLATE.md
- Makefile.d/dependencies.mk
- Makefile.d/functions.mk
- apis/proto/v1/manager/index/index_manager.proto
- apis/proto/v1/payload/payload.proto
- apis/swagger/v1/manager/index/index_manager.swagger.json
- internal/info/info.go
- internal/net/grpc/errdetails/errdetails.go
- internal/observability/trace/status.go
- pkg/agent/core/faiss/handler/grpc/insert.go
- pkg/agent/core/faiss/handler/grpc/remove.go
- pkg/agent/core/faiss/handler/grpc/search.go
- pkg/agent/core/faiss/handler/grpc/update.go
- pkg/agent/core/ngt/handler/grpc/flush.go
- pkg/agent/core/ngt/handler/grpc/insert.go
- pkg/agent/core/ngt/handler/grpc/linear_search.go
- pkg/agent/core/ngt/handler/grpc/remove.go
- pkg/agent/core/ngt/handler/grpc/search.go
- pkg/agent/core/ngt/handler/grpc/update.go
- pkg/manager/index/handler/grpc/handler.go
- pkg/manager/index/handler/grpc/handler_test.go
- pkg/manager/index/service/indexer_test.go
- pkg/manager/index/service/option.go
- pkg/manager/index/usecase/indexer.go
- rust/libs/proto/src/manager.index.v1.tonic.rs
- rust/libs/proto/src/payload.v1.rs
- versions/GOLANGCILINT_VERSION
- versions/K3S_VERSION
- versions/NGT_VERSION
- versions/PROMETHEUS_STACK_VERSION
- versions/YQ_VERSION
Additional comments not posted (5)
.github/workflows/release.yml (1)
64-64: Ensure compatibility withsoftprops/action-gh-release@v2.The update from
softprops/action-gh-release@v1tov2should be verified for compatibility. Check the release notes for any breaking changes or required modifications.pkg/manager/index/service/indexer.go (1)
56-59: Ensure thread safety withsync.Map.Verify that the usage of
sync.MapforshouldSaveListandindexInfosensures thread safety and does not introduce any race conditions.apis/docs/v1/docs.md (2)
299-310: The addition of theInfo.Index.Detailsection looks good and provides clear information about the index count for each agent.
311-319: The addition of theInfo.Index.Detail.CountsEntrysection looks good and provides clear information about the counts entry for each agent.pkg/agent/core/ngt/service/ngt_test.go (1)
1615-1616: Ensure the test client is correctly initialized.Verify that the
grpc.Newfunction correctly initializes the client with the provided options. Ensure that theWithInsecureoption is appropriate for the test context.
pkg/manager/index/service/indexer.go
Outdated
| return err | ||
| } | ||
| return nil | ||
| } | ||
| var mu sync.Mutex |
There was a problem hiding this comment.
Avoid potential deadlock with mu.
Using mu.Lock() without a timeout or context check can lead to potential deadlocks. Consider using mu.TryLock() with a timeout or context check.
pkg/manager/index/service/indexer.go
Outdated
| func (idx *index) saveIndex(ctx context.Context, force bool) (err error) { | ||
| ctx, span := trace.StartSpan(ctx, "vald/manager-index/service/Indexer.saveIndex") |
There was a problem hiding this comment.
Handle potential nil pointer dereference.
Ensure that idx.client.GetClient() and idx.client.GetAddrs(ctx) do not return nil to avoid potential nil pointer dereference.
| @@ -38,6 +38,7 @@ import ( | |||
| "github.com/vdaas/vald/internal/file" | |||
| kvald "github.com/vdaas/vald/internal/k8s/vald" | |||
| "github.com/vdaas/vald/internal/log" | |||
| "github.com/vdaas/vald/internal/net/grpc" | |||
There was a problem hiding this comment.
Remove unused imports.
The import statements for file, kvald, log, grpc, safety, strings, and sync should be verified for usage within the file. Unused imports should be removed to keep the code clean and maintainable.
| addr string | ||
| client grpc.Client |
There was a problem hiding this comment.
Fix the type mismatch in test arguments.
The requests field in the args struct is of type []*payload.Upsert_MultiRequest, but the MultiInsert method expects *payload.Insert_MultiRequest. This type mismatch needs to be addressed to prevent runtime errors.
- requests []*payload.Upsert_MultiRequest
+ requests []*payload.Insert_MultiRequestCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| addr string | |
| client grpc.Client | |
| addr string | |
| client grpc.Client |
|
|
||
| defer test.args.client.Close(ctx) | ||
|
|
||
| for i := 0; i < 2; i++ { | ||
| for _, req := range test.args.requests { | ||
| _, err := client.MultiUpsert(ctx, req) | ||
| _, err := test.args.client.Do(ctx, test.args.addr, | ||
| func(ctx context.Context, conn *grpc.ClientConn, opts ...grpc.CallOption) (any, error) { | ||
| return vald.NewValdClient(conn).MultiInsert(ctx, req) | ||
| }) |
There was a problem hiding this comment.
Fix the type mismatch in gRPC call.
The MultiInsert method expects *payload.Insert_MultiRequest, but the test is passing *payload.Upsert_MultiRequest. This type mismatch needs to be addressed to prevent runtime errors.
- return vald.NewValdClient(conn).MultiInsert(ctx, req)
+ return vald.NewValdClient(conn).MultiInsert(ctx, req.(*payload.Insert_MultiRequest))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| defer test.args.client.Close(ctx) | |
| for i := 0; i < 2; i++ { | |
| for _, req := range test.args.requests { | |
| _, err := client.MultiUpsert(ctx, req) | |
| _, err := test.args.client.Do(ctx, test.args.addr, | |
| func(ctx context.Context, conn *grpc.ClientConn, opts ...grpc.CallOption) (any, error) { | |
| return vald.NewValdClient(conn).MultiInsert(ctx, req) | |
| }) | |
| defer test.args.client.Close(ctx) | |
| for i := 0; i < 2; i++ { | |
| for _, req := range test.args.requests { | |
| _, err := test.args.client.Do(ctx, test.args.addr, | |
| func(ctx context.Context, conn *grpc.ClientConn, opts ...grpc.CallOption) (any, error) { | |
| return vald.NewValdClient(conn).MultiInsert(ctx, req.(*payload.Insert_MultiRequest)) | |
| }) |
kpango
force-pushed
the
refactor/manager-index/small-refactor
branch
from
189ec53
to
9a570ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Out of diff range and nitpick comments (1)
pkg/agent/core/ngt/service/ngt_test.go (1)
Line range hint
45-47: Remove unused imports.The import statements for
kvs,metadata, andvqueueshould be verified for usage within the file. Unused imports should be removed to keep the code clean and maintainable.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (10)
apis/grpc/v1/manager/index/index_manager.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/manager/index/index_manager_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.json.gois excluded by!**/*.pb.json.goapis/grpc/v1/payload/payload_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goexample/client/go.modis excluded by!**/*.modexample/client/go.sumis excluded by!**/*.sumgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sumrust/Cargo.lockis excluded by!**/*.lock
Files selected for processing (39)
- .github/ISSUE_TEMPLATE/bug_report.md (1 hunks)
- .github/ISSUE_TEMPLATE/security_issue_report.md (1 hunks)
- .github/PULL_REQUEST_TEMPLATE.md (1 hunks)
- .github/workflows/release.yml (1 hunks)
- Makefile.d/dependencies.mk (1 hunks)
- Makefile.d/functions.mk (1 hunks)
- apis/docs/v1/docs.md (3 hunks)
- apis/proto/v1/manager/index/index_manager.proto (1 hunks)
- apis/proto/v1/payload/payload.proto (1 hunks)
- apis/swagger/v1/manager/index/index_manager.swagger.json (2 hunks)
- internal/info/info.go (5 hunks)
- internal/net/grpc/errdetails/errdetails.go (2 hunks)
- internal/observability/trace/status.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/insert.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/remove.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/search.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/update.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/flush.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/insert.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/linear_search.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/remove.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/search.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/update.go (1 hunks)
- pkg/agent/core/ngt/service/ngt_test.go (5 hunks)
- pkg/manager/index/handler/grpc/handler.go (1 hunks)
- pkg/manager/index/handler/grpc/handler_test.go (1 hunks)
- pkg/manager/index/service/indexer.go (8 hunks)
- pkg/manager/index/service/indexer_test.go (1 hunks)
- pkg/manager/index/service/option.go (4 hunks)
- pkg/manager/index/usecase/indexer.go (1 hunks)
- rust/libs/proto/src/manager.index.v1.tonic.rs (5 hunks)
- rust/libs/proto/src/payload.v1.rs (1 hunks)
- versions/GOLANGCILINT_VERSION (1 hunks)
- versions/K3S_VERSION (1 hunks)
- versions/KIND_VERSION (1 hunks)
- versions/NGT_VERSION (1 hunks)
- versions/PROMETHEUS_STACK_VERSION (1 hunks)
- versions/YQ_VERSION (1 hunks)
- versions/actions/SOFTPROPS_ACTION_GH_RELEASE (1 hunks)
Files skipped from review due to trivial changes (1)
- versions/KIND_VERSION
Files skipped from review as they are similar to previous changes (35)
- .github/ISSUE_TEMPLATE/bug_report.md
- .github/ISSUE_TEMPLATE/security_issue_report.md
- .github/PULL_REQUEST_TEMPLATE.md
- .github/workflows/release.yml
- Makefile.d/dependencies.mk
- Makefile.d/functions.mk
- apis/proto/v1/manager/index/index_manager.proto
- apis/proto/v1/payload/payload.proto
- apis/swagger/v1/manager/index/index_manager.swagger.json
- internal/info/info.go
- internal/net/grpc/errdetails/errdetails.go
- internal/observability/trace/status.go
- pkg/agent/core/faiss/handler/grpc/insert.go
- pkg/agent/core/faiss/handler/grpc/remove.go
- pkg/agent/core/faiss/handler/grpc/search.go
- pkg/agent/core/faiss/handler/grpc/update.go
- pkg/agent/core/ngt/handler/grpc/flush.go
- pkg/agent/core/ngt/handler/grpc/insert.go
- pkg/agent/core/ngt/handler/grpc/linear_search.go
- pkg/agent/core/ngt/handler/grpc/remove.go
- pkg/agent/core/ngt/handler/grpc/search.go
- pkg/agent/core/ngt/handler/grpc/update.go
- pkg/manager/index/handler/grpc/handler.go
- pkg/manager/index/handler/grpc/handler_test.go
- pkg/manager/index/service/indexer_test.go
- pkg/manager/index/service/option.go
- pkg/manager/index/usecase/indexer.go
- rust/libs/proto/src/manager.index.v1.tonic.rs
- rust/libs/proto/src/payload.v1.rs
- versions/GOLANGCILINT_VERSION
- versions/K3S_VERSION
- versions/NGT_VERSION
- versions/PROMETHEUS_STACK_VERSION
- versions/YQ_VERSION
- versions/actions/SOFTPROPS_ACTION_GH_RELEASE
Additional comments not posted (9)
pkg/manager/index/service/indexer.go (4)
46-47: New methodsIsSavingandLoadIndexDetailadded to theIndexerinterface look good and align with the existing design.
56-70: The new fields added to theindexstruct are necessary for the added functionality and are appropriately named.
79-80: The initialization of thesavingfield and the duration checks in theNewfunction are correctly implemented.
219-251: The changes to thecreateIndexmethod are correctly implemented. Ensure thatidx.client.GetClient()andidx.client.GetAddrs(ctx)do not return nil to avoid potential nil pointer dereference.apis/docs/v1/docs.md (1)
313-319: LGTM!pkg/agent/core/ngt/service/ngt_test.go (4)
41-41: Remove unused imports.The import statements for
file,kvald,log,safety,strings, andsyncshould be verified for usage within the file. Unused imports should be removed to keep the code clean and maintainable.
1559-1560: Fix the type mismatch in test arguments.The
requestsfield in theargsstruct is of type[]*payload.Upsert_MultiRequest, but theMultiInsertmethod expects*payload.Insert_MultiRequest. This type mismatch needs to be addressed to prevent runtime errors.- requests []*payload.Upsert_MultiRequest + requests []*payload.Insert_MultiRequest
1615-1616: Ensure secure initialization of gRPC client.Using
grpc.WithInsecure(true)can expose the client to security risks. Consider using secure options for initializing the gRPC client.
1638-1646: Fix the type mismatch in gRPC call.The
MultiInsertmethod expects*payload.Insert_MultiRequest, but the test is passing*payload.Upsert_MultiRequest. This type mismatch needs to be addressed to prevent runtime errors.- return vald.NewValdClient(conn).MultiInsert(ctx, req) + return vald.NewValdClient(conn).MultiInsert(ctx, req.(*payload.Insert_MultiRequest))
|
@coderabbitai can you please fix info.go's test code? |
|
Tip For best results, initiate chat on the files or code changes. @kpango, I have reviewed the Updated
|
kpango
force-pushed
the
refactor/manager-index/small-refactor
branch
from
9a570ca
to
8aa8c98
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (10)
apis/grpc/v1/manager/index/index_manager.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/manager/index/index_manager_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.json.gois excluded by!**/*.pb.json.goapis/grpc/v1/payload/payload_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goexample/client/go.modis excluded by!**/*.modexample/client/go.sumis excluded by!**/*.sumgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sumrust/Cargo.lockis excluded by!**/*.lock
Files selected for processing (41)
- .github/ISSUE_TEMPLATE/bug_report.md (1 hunks)
- .github/ISSUE_TEMPLATE/security_issue_report.md (1 hunks)
- .github/PULL_REQUEST_TEMPLATE.md (1 hunks)
- .github/workflows/release.yml (1 hunks)
- Makefile.d/dependencies.mk (1 hunks)
- Makefile.d/functions.mk (1 hunks)
- apis/docs/v1/docs.md (3 hunks)
- apis/proto/v1/manager/index/index_manager.proto (1 hunks)
- apis/proto/v1/payload/payload.proto (1 hunks)
- apis/swagger/v1/manager/index/index_manager.swagger.json (2 hunks)
- hack/go.mod.default (1 hunks)
- internal/info/info.go (5 hunks)
- internal/net/grpc/errdetails/errdetails.go (2 hunks)
- internal/net/http/metrics/pprof.go (2 hunks)
- internal/observability/trace/status.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/insert.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/remove.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/search.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/update.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/flush.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/insert.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/linear_search.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/remove.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/search.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/update.go (1 hunks)
- pkg/agent/core/ngt/service/ngt_test.go (5 hunks)
- pkg/manager/index/handler/grpc/handler.go (1 hunks)
- pkg/manager/index/handler/grpc/handler_test.go (1 hunks)
- pkg/manager/index/service/indexer.go (8 hunks)
- pkg/manager/index/service/indexer_test.go (1 hunks)
- pkg/manager/index/service/option.go (4 hunks)
- pkg/manager/index/usecase/indexer.go (1 hunks)
- rust/libs/proto/src/manager.index.v1.tonic.rs (5 hunks)
- rust/libs/proto/src/payload.v1.rs (1 hunks)
- versions/GOLANGCILINT_VERSION (1 hunks)
- versions/K3S_VERSION (1 hunks)
- versions/KIND_VERSION (1 hunks)
- versions/NGT_VERSION (1 hunks)
- versions/PROMETHEUS_STACK_VERSION (1 hunks)
- versions/YQ_VERSION (1 hunks)
- versions/actions/SOFTPROPS_ACTION_GH_RELEASE (1 hunks)
Files skipped from review due to trivial changes (1)
- hack/go.mod.default
Files skipped from review as they are similar to previous changes (36)
- .github/ISSUE_TEMPLATE/bug_report.md
- .github/ISSUE_TEMPLATE/security_issue_report.md
- .github/PULL_REQUEST_TEMPLATE.md
- .github/workflows/release.yml
- Makefile.d/dependencies.mk
- Makefile.d/functions.mk
- apis/proto/v1/manager/index/index_manager.proto
- apis/proto/v1/payload/payload.proto
- apis/swagger/v1/manager/index/index_manager.swagger.json
- internal/info/info.go
- internal/net/grpc/errdetails/errdetails.go
- internal/observability/trace/status.go
- pkg/agent/core/faiss/handler/grpc/insert.go
- pkg/agent/core/faiss/handler/grpc/remove.go
- pkg/agent/core/faiss/handler/grpc/search.go
- pkg/agent/core/faiss/handler/grpc/update.go
- pkg/agent/core/ngt/handler/grpc/flush.go
- pkg/agent/core/ngt/handler/grpc/insert.go
- pkg/agent/core/ngt/handler/grpc/linear_search.go
- pkg/agent/core/ngt/handler/grpc/remove.go
- pkg/agent/core/ngt/handler/grpc/search.go
- pkg/agent/core/ngt/handler/grpc/update.go
- pkg/manager/index/handler/grpc/handler.go
- pkg/manager/index/handler/grpc/handler_test.go
- pkg/manager/index/service/indexer_test.go
- pkg/manager/index/service/option.go
- pkg/manager/index/usecase/indexer.go
- rust/libs/proto/src/manager.index.v1.tonic.rs
- rust/libs/proto/src/payload.v1.rs
- versions/GOLANGCILINT_VERSION
- versions/K3S_VERSION
- versions/KIND_VERSION
- versions/NGT_VERSION
- versions/PROMETHEUS_STACK_VERSION
- versions/YQ_VERSION
- versions/actions/SOFTPROPS_ACTION_GH_RELEASE
Additional comments not posted (14)
internal/net/http/metrics/pprof.go (4)
25-25: Ensure thepyprofimport is necessary and correctly used.Verify that the
pyprofimport is used correctly in the file.
120-127: The route for delta heap profile is correctly defined. Ensure the handlerpyprof.Heapis appropriate and correctly implemented.Verify the correctness of the handler
pyprof.Heap.
128-135: The route for delta mutex profile is correctly defined. Ensure the handlerpyprof.Mutexis appropriate and correctly implemented.Verify the correctness of the handler
pyprof.Mutex.
136-143: The route for delta block profile is correctly defined. Ensure the handlerpyprof.Blockis appropriate and correctly implemented.Verify the correctness of the handler
pyprof.Block.pkg/manager/index/service/indexer.go (3)
46-47: The methodsIsSavingandLoadIndexDetailare correctly defined in theIndexerinterface. Ensure their implementations are correct.Verify the correctness of the implementations of
IsSavingandLoadIndexDetail.
56-63: The new fields in theindexstruct are correctly defined. Ensure their usage throughout the code is appropriate and consistent.Verify the correctness of the usage of the new fields in the
indexstruct.
79-80: The new fields are correctly initialized in theNewfunction. Ensure all edge cases are handled.Verify the correctness of the initialization of the new fields in the
Newfunction.apis/docs/v1/docs.md (1)
26-27: Addition to the table of contents forInfo.Index.Detaillooks good.pkg/agent/core/ngt/service/ngt_test.go (6)
41-41: Remove unused imports.The import statements for
file,kvald,log,safety,strings, andsyncshould be verified for usage within the file. Unused imports should be removed to keep the code clean and maintainable.
42-44: Verify the usage of additional imports.Ensure that the imported packages
kvs,metadata, andvqueueare used within the file. Unused imports should be removed to maintain code cleanliness.
1559-1560: Fix the type mismatch in test arguments.The
requestsfield in theargsstruct is of type[]*payload.Upsert_MultiRequest, but theMultiInsertmethod expects*payload.Insert_MultiRequest. This type mismatch needs to be addressed to prevent runtime errors.- requests []*payload.Upsert_MultiRequest + requests []*payload.Insert_MultiRequest
1615-1616: Initialization looks correct.The
addrandclientfields are correctly initialized in the test case.
1645-1645: The type mismatch issue withMultiInsertexpecting anInsert_MultiRequestbut receiving anUpsert_MultiRequeststill persists. This needs to be addressed to prevent runtime errors.
1638-1646: Fix the type mismatch in gRPC call.The
MultiInsertmethod expects*payload.Insert_MultiRequest, but the test is passing*payload.Upsert_MultiRequest. This type mismatch needs to be addressed to prevent runtime errors.- return vald.NewValdClient(conn).MultiInsert(ctx, req) + return vald.NewValdClient(conn).MultiInsert(ctx, req.(*payload.Insert_MultiRequest))
apis/docs/v1/docs.md
Outdated
| <a name="payload-v1-Info-Index-Detail"></a> | ||
|
|
||
| ### Info.Index.Detail | ||
|
|
||
| Represent the index count for each Agents message. | ||
|
|
||
| | Field | Type | Label | Description | | ||
| | ----------- | -------------------------------------------------------------------------- | -------- | ---------------------------------- | | ||
| | counts | [Info.Index.Detail.CountsEntry](#payload-v1-Info-Index-Detail-CountsEntry) | repeated | count infos for each agents | | ||
| | replica | [uint32](#uint32) | | index replica of vald cluster | | ||
| | live_agents | [uint32](#uint32) | | live agent replica of vald cluster | | ||
|
|
||
| <a name="payload-v1-Info-Index-Detail-CountsEntry"></a> | ||
|
|
||
| ### Info.Index.Detail.CountsEntry | ||
|
|
||
| | Field | Type | Label | Description | | ||
| | ----- | ------------------------------------------------ | ----- | ----------- | | ||
| | key | [string](#string) | | | | ||
| | value | [Info.Index.Count](#payload-v1-Info-Index-Count) | | | | ||
|
|
There was a problem hiding this comment.
The new messages Info.Index.Detail and CountsEntry are well-defined and follow the existing structure. However, there is a grammatical issue in the description.
- count infos for each agents
+ count info for each agentCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| <a name="payload-v1-Info-Index-Detail"></a> | |
| ### Info.Index.Detail | |
| Represent the index count for each Agents message. | |
| | Field | Type | Label | Description | | |
| | ----------- | -------------------------------------------------------------------------- | -------- | ---------------------------------- | | |
| | counts | [Info.Index.Detail.CountsEntry](#payload-v1-Info-Index-Detail-CountsEntry) | repeated | count infos for each agents | | |
| | replica | [uint32](#uint32) | | index replica of vald cluster | | |
| | live_agents | [uint32](#uint32) | | live agent replica of vald cluster | | |
| <a name="payload-v1-Info-Index-Detail-CountsEntry"></a> | |
| ### Info.Index.Detail.CountsEntry | |
| | Field | Type | Label | Description | | |
| | ----- | ------------------------------------------------ | ----- | ----------- | | |
| | key | [string](#string) | | | | |
| | value | [Info.Index.Count](#payload-v1-Info-Index-Count) | | | | |
| <a name="payload-v1-Info-Index-Detail"></a> | |
| ### Info.Index.Detail | |
| Represent the index count for each Agents message. | |
| | Field | Type | Label | Description | | |
| | ----------- | -------------------------------------------------------------------------- | -------- | ---------------------------------- | | |
| | counts | [Info.Index.Detail.CountsEntry](#payload-v1-Info-Index-Detail-CountsEntry) | repeated | count info for each agent | | |
| | replica | [uint32](#uint32) | | index replica of vald cluster | | |
| | live_agents | [uint32](#uint32) | | live agent replica of vald cluster | | |
| <a name="payload-v1-Info-Index-Detail-CountsEntry"></a> | |
| ### Info.Index.Detail.CountsEntry | |
| | Field | Type | Label | Description | | |
| | ----- | ------------------------------------------------ | ----- | ----------- | | |
| | key | [string](#string) | | | | |
| | value | [Info.Index.Count](#payload-v1-Info-Index-Count) | | | | |
pkg/manager/index/service/indexer.go
Outdated
| func (idx *index) saveIndex(ctx context.Context, force bool) (err error) { | ||
| ctx, span := trace.StartSpan(ctx, "vald/manager-index/service/Indexer.saveIndex") | ||
| defer func() { | ||
| if span != nil { | ||
| span.End() | ||
| } | ||
| timer.Stop() | ||
| }() | ||
|
|
||
| if idx.saving.Load() { | ||
| return nil | ||
| } | ||
| idx.saving.Store(true) | ||
| defer idx.saving.Store(false) | ||
| return idx.client.GetClient().OrderedRangeConcurrent(ctx, idx.client.GetAddrs(ctx), | ||
| idx.saveIndexConcurrency, | ||
| func(ctx context.Context, | ||
| addr string, conn *grpc.ClientConn, copts ...grpc.CallOption, | ||
| ) (err error) { | ||
| _, ok := idx.shouldSaveList.LoadAndDelete(addr) | ||
| if !ok && !force { | ||
| return nil | ||
| } | ||
| _, err = agent.NewAgentClient(conn).SaveIndex(ctx, new(payload.Empty), copts...) | ||
| if err != nil { | ||
| st, ok := status.FromError(err) | ||
| if ok && st != nil && st.Code() == codes.FailedPrecondition { | ||
| log.Debugf("CreateIndex of %s skipped, message: %s, err: %v", addr, st.Message(), errors.Join(st.Err(), err)) | ||
| return nil | ||
| } | ||
| log.Warnf("an error occurred while calling CreateIndex of %s: %s", addr, err) | ||
| return err | ||
| } | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
The changes to the saveIndex method are correctly implemented. However, ensure that idx.client.GetClient() and idx.client.GetAddrs(ctx) do not return nil to avoid potential nil pointer dereference.
Ensure that idx.client.GetClient() and idx.client.GetAddrs(ctx) do not return nil to avoid potential nil pointer dereference.
pkg/manager/index/service/indexer.go
Outdated
| return err | ||
| } | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
Ensure that infoMap.Load(addr) does not return nil before storing it in idx.indexInfos to avoid potential nil pointer dereference.
Ensure that infoMap.Load(addr) does not return nil before storing it in idx.indexInfos to avoid potential nil pointer dereference.
kpango
force-pushed
the
refactor/manager-index/small-refactor
branch
from
640f8e9
to
8f35f90
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (10)
apis/grpc/v1/manager/index/index_manager.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/manager/index/index_manager_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.json.gois excluded by!**/*.pb.json.goapis/grpc/v1/payload/payload_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goexample/client/go.modis excluded by!**/*.modexample/client/go.sumis excluded by!**/*.sumgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sumrust/Cargo.lockis excluded by!**/*.lock
Files selected for processing (42)
- .github/ISSUE_TEMPLATE/bug_report.md (1 hunks)
- .github/ISSUE_TEMPLATE/security_issue_report.md (1 hunks)
- .github/PULL_REQUEST_TEMPLATE.md (1 hunks)
- .github/workflows/release.yml (1 hunks)
- Makefile.d/dependencies.mk (1 hunks)
- Makefile.d/functions.mk (1 hunks)
- apis/docs/v1/docs.md (3 hunks)
- apis/proto/v1/manager/index/index_manager.proto (1 hunks)
- apis/proto/v1/payload/payload.proto (1 hunks)
- apis/swagger/v1/manager/index/index_manager.swagger.json (2 hunks)
- hack/go.mod.default (1 hunks)
- internal/info/info.go (5 hunks)
- internal/net/grpc/errdetails/errdetails.go (2 hunks)
- internal/net/http/metrics/pprof.go (2 hunks)
- internal/observability/trace/status.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/insert.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/remove.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/search.go (1 hunks)
- pkg/agent/core/faiss/handler/grpc/update.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/flush.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/insert.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/linear_search.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/remove.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/search.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/update.go (1 hunks)
- pkg/agent/core/ngt/service/ngt_test.go (5 hunks)
- pkg/manager/index/handler/grpc/handler.go (1 hunks)
- pkg/manager/index/handler/grpc/handler_test.go (1 hunks)
- pkg/manager/index/service/indexer.go (8 hunks)
- pkg/manager/index/service/indexer_test.go (1 hunks)
- pkg/manager/index/service/option.go (4 hunks)
- pkg/manager/index/usecase/indexer.go (1 hunks)
- rust/libs/proto/src/manager.index.v1.tonic.rs (5 hunks)
- rust/libs/proto/src/payload.v1.rs (1 hunks)
- versions/GOLANGCILINT_VERSION (1 hunks)
- versions/K3S_VERSION (1 hunks)
- versions/KIND_VERSION (1 hunks)
- versions/NGT_VERSION (1 hunks)
- versions/PROMETHEUS_STACK_VERSION (1 hunks)
- versions/YQ_VERSION (1 hunks)
- versions/actions/CODECOV_CODECOV_ACTION (1 hunks)
- versions/actions/SOFTPROPS_ACTION_GH_RELEASE (1 hunks)
Files skipped from review due to trivial changes (1)
- versions/actions/CODECOV_CODECOV_ACTION
Files skipped from review as they are similar to previous changes (38)
- .github/ISSUE_TEMPLATE/bug_report.md
- .github/ISSUE_TEMPLATE/security_issue_report.md
- .github/PULL_REQUEST_TEMPLATE.md
- .github/workflows/release.yml
- Makefile.d/dependencies.mk
- Makefile.d/functions.mk
- apis/proto/v1/manager/index/index_manager.proto
- apis/proto/v1/payload/payload.proto
- apis/swagger/v1/manager/index/index_manager.swagger.json
- hack/go.mod.default
- internal/info/info.go
- internal/net/grpc/errdetails/errdetails.go
- internal/net/http/metrics/pprof.go
- internal/observability/trace/status.go
- pkg/agent/core/faiss/handler/grpc/insert.go
- pkg/agent/core/faiss/handler/grpc/remove.go
- pkg/agent/core/faiss/handler/grpc/search.go
- pkg/agent/core/faiss/handler/grpc/update.go
- pkg/agent/core/ngt/handler/grpc/flush.go
- pkg/agent/core/ngt/handler/grpc/insert.go
- pkg/agent/core/ngt/handler/grpc/linear_search.go
- pkg/agent/core/ngt/handler/grpc/remove.go
- pkg/agent/core/ngt/handler/grpc/search.go
- pkg/agent/core/ngt/handler/grpc/update.go
- pkg/manager/index/handler/grpc/handler.go
- pkg/manager/index/handler/grpc/handler_test.go
- pkg/manager/index/service/indexer_test.go
- pkg/manager/index/service/option.go
- pkg/manager/index/usecase/indexer.go
- rust/libs/proto/src/manager.index.v1.tonic.rs
- rust/libs/proto/src/payload.v1.rs
- versions/GOLANGCILINT_VERSION
- versions/K3S_VERSION
- versions/KIND_VERSION
- versions/NGT_VERSION
- versions/PROMETHEUS_STACK_VERSION
- versions/YQ_VERSION
- versions/actions/SOFTPROPS_ACTION_GH_RELEASE
Additional comments not posted (10)
pkg/manager/index/service/indexer.go (5)
46-47: New methodsIsSavingandLoadIndexDetailare correctly added to theIndexerinterface.
56-70: New fieldssaveIndexDuration,saveIndexDurationLimit,shouldSaveList,saveIndexConcurrency,indexing,saving, andemptyare correctly added to theindexstruct.
79-80: The initialization of the new fieldsindexingandsavingin theNewfunction is correctly implemented.
254-255: Ensure thatidx.client.GetClient()andidx.client.GetAddrs(ctx)do not return nil to avoid potential nil pointer dereference.
287-287: Ensure thatinfoMap.Load(addr)does not return nil before storing it inidx.indexInfosto avoid potential nil pointer dereference.apis/docs/v1/docs.md (1)
313-319: The new messageInfo.Index.Detail.CountsEntryis well-defined and follows the existing structure.pkg/agent/core/ngt/service/ngt_test.go (4)
41-41: Remove unused imports.The import statements for
file,kvald,log,grpc,safety,strings, andsyncshould be verified for usage within the file. Unused imports should be removed to keep the code clean and maintainable.
1559-1560: Fix the type mismatch in test arguments.The
requestsfield in theargsstruct is of type[]*payload.Upsert_MultiRequest, but theMultiInsertmethod expects*payload.Insert_MultiRequest. This type mismatch needs to be addressed to prevent runtime errors.- requests []*payload.Upsert_MultiRequest + requests []*payload.Insert_MultiRequest
1615-1616: Verify the initialization of the gRPC client.The
clientis initialized withgrpc.New(grpc.WithInsecure(true)). Ensure that this initialization is correct and secure for the test environment.
1638-1646: Fix the type mismatch in gRPC call.The
MultiInsertmethod expects*payload.Insert_MultiRequest, but the test is passing*payload.Upsert_MultiRequest. This type mismatch needs to be addressed to prevent runtime errors.- return vald.NewValdClient(conn).MultiInsert(ctx, req) + return vald.NewValdClient(conn).MultiInsert(ctx, req.(*payload.Insert_MultiRequest))
apis/docs/v1/docs.md
Outdated
|
|
||
| | Field | Type | Label | Description | | ||
| | ----------- | -------------------------------------------------------------------------- | -------- | ---------------------------------- | | ||
| | counts | [Info.Index.Detail.CountsEntry](#payload-v1-Info-Index-Detail-CountsEntry) | repeated | count infos for each agents | |
There was a problem hiding this comment.
Use singular form for "agents".
- count infos for each agents
+ count info for each agentCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| | counts | [Info.Index.Detail.CountsEntry](#payload-v1-Info-Index-Detail-CountsEntry) | repeated | count infos for each agents | | |
| | counts | [Info.Index.Detail.CountsEntry](#payload-v1-Info-Index-Detail-CountsEntry) | repeated | count info for each agent | |
apis/docs/v1/docs.md
Outdated
| | Method Name | Request Type | Response Type | Description | | ||
| | ----------- | -------------------------------------- | -------------------------------------------------------------- | --------------------------------------------------------------- | | ||
| | IndexInfo | [.payload.v1.Empty](#payload-v1-Empty) | [.payload.v1.Info.Index.Count](#payload-v1-Info-Index-Count) | Represent the RPC to get the index information. | | ||
| | IndexDetail | [.payload.v1.Empty](#payload-v1-Empty) | [.payload.v1.Info.Index.Detail](#payload-v1-Info-Index-Detail) | Represent the RPC to get the index information for each agents. | |
There was a problem hiding this comment.
Use singular form for "agents".
- index information for each agents
+ index information for each agentCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| | IndexDetail | [.payload.v1.Empty](#payload-v1-Empty) | [.payload.v1.Info.Index.Detail](#payload-v1-Info-Index-Detail) | Represent the RPC to get the index information for each agents. | | |
| | IndexDetail | [.payload.v1.Empty](#payload-v1-Empty) | [.payload.v1.Info.Index.Detail](#payload-v1-Info-Index-Detail) | Represent the RPC to get the index information for each agent. | |
| @@ -38,6 +38,7 @@ import ( | |||
| "github.com/vdaas/vald/internal/file" | |||
| kvald "github.com/vdaas/vald/internal/k8s/vald" | |||
| "github.com/vdaas/vald/internal/log" | |||
| "github.com/vdaas/vald/internal/net/grpc" | |||
| "github.com/vdaas/vald/internal/safety" | |||
There was a problem hiding this comment.
Remove unused imports.
The import statements for kvs, metadata, and vqueue should be verified for usage within the file. Unused imports should be removed to keep the code clean and maintainable.
pkg/manager/index/service/indexer.go
Outdated
| if idx.saveIndexDuration <= 0 { | ||
| idx.saveIndexDuration = math.MaxInt64 | ||
| } |
There was a problem hiding this comment.
Consider using mu.TryLock() with a timeout or context check to avoid potential deadlocks.
- if !mu.TryLock() {
+ if !mu.TryLockWithTimeout(ctx, timeout) {
return
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if idx.saveIndexDuration <= 0 { | |
| idx.saveIndexDuration = math.MaxInt64 | |
| } | |
| if idx.saveIndexDuration <= 0 { | |
| idx.saveIndexDuration = math.MaxInt64 | |
| } | |
| if !mu.TryLockWithTimeout(ctx, timeout) { | |
| return | |
| } |
pkg/manager/index/service/indexer.go
Outdated
| return errors.Join(idx.client.GetClient().OrderedRangeConcurrent(ctx, idx.client.GetAddrs(ctx), | ||
| idx.createIndexConcurrency, |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure that idx.client.GetClient() and idx.client.GetAddrs(ctx) do not return nil to avoid potential nil pointer dereference. The GetAddrs method can return nil if net.DefaultResolver.LookupIPAddr fails, which needs to be handled properly.
internal/client/v1/client/discoverer/discover.go:182:return nilifLookupIPAddrfails.
Analysis chain
Ensure that idx.client.GetClient() and idx.client.GetAddrs(ctx) do not return nil to avoid potential nil pointer dereference.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `idx.client.GetClient()` and `idx.client.GetAddrs(ctx)` do not return nil.
# Test: Search for the function usage. Expect: No nil returns.
rg --type go 'idx.client.GetClient()' --context 5
rg --type go 'idx.client.GetAddrs(ctx)' --context 5
Length of output: 5581
Script:
#!/bin/bash
# Locate the implementation of the GetClient method
ast-grep --lang go --pattern 'func ($_ $_) GetClient() $_ { $$$ }'
# Locate the implementation of the GetAddrs method
ast-grep --lang go --pattern 'func ($_ $_) GetAddrs($_) $_ { $$$ }'
Length of output: 2599
Script:
#!/bin/bash
# Locate the initialization of `c.client` in discover.go
rg --type go 'c.client =' --context 5
# Locate the usage of `net.DefaultResolver.LookupIPAddr` in discover.go to ensure proper error handling
rg --type go 'net.DefaultResolver.LookupIPAddr' --context 5
Length of output: 3999
pkg/manager/index/service/indexer.go
Outdated
| func (idx *index) LoadIndexDetail() (detail *payload.Info_Index_Detail) { | ||
| detail = new(payload.Info_Index_Detail) | ||
| idx.indexInfos.Range(func(addr string, info *payload.Info_Index_Count) bool { | ||
| detail.Counts[addr] = info | ||
| return true | ||
| }) | ||
| return detail | ||
| } |
There was a problem hiding this comment.
Ensure that detail.Counts is initialized before storing values to avoid potential nil pointer dereference.
func (idx *index) LoadIndexDetail() (detail *payload.Info_Index_Detail) {
+ detail = &payload.Info_Index_Detail{
+ Counts: make(map[string]*payload.Info_Index_Count),
+ }
idx.indexInfos.Range(func(addr string, info *payload.Info_Index_Count) bool {
detail.Counts[addr] = info
return true
})
return detail
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (idx *index) LoadIndexDetail() (detail *payload.Info_Index_Detail) { | |
| detail = new(payload.Info_Index_Detail) | |
| idx.indexInfos.Range(func(addr string, info *payload.Info_Index_Count) bool { | |
| detail.Counts[addr] = info | |
| return true | |
| }) | |
| return detail | |
| } | |
| func (idx *index) LoadIndexDetail() (detail *payload.Info_Index_Detail) { | |
| detail = &payload.Info_Index_Detail{ | |
| Counts: make(map[string]*payload.Info_Index_Count), | |
| } | |
| idx.indexInfos.Range(func(addr string, info *payload.Info_Index_Count) bool { | |
| detail.Counts[addr] = info | |
| return true | |
| }) | |
| return detail | |
| } |
kpango
force-pushed
the
refactor/manager-index/small-refactor
branch
from
8f35f90
to
8a36249
Compare
Description:
Related Issue:
Versions:
Checklist:
Special notes for your reviewer:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores