Refctor for release v1.7.14 #2639

kpango · 2024-09-21T05:50:37Z

Description

This PR includes Six refactorings

Remove grpc-go's deprecated interface. (DialContext --> NewClient) Please Refer Here
Add imagePullPolicy to initContainers configuration.
Refactor Usearch installation script for multiple environments.
Update Dependencies.
Readable AccessLog Interceptor.
Reduced inclusion of gRPC Richer Error Information in Status
a. Expose error detail TypeNames
b. Add error detail merge functionality to status.WithDetails function.

Related Issue

Versions

Vald Version: v1.7.13
Go Version: v1.23.1
Rust Version: v1.81.0
Docker Version: v27.2.1
Kubernetes Version: v1.31.0
Helm Version: v3.16.0
NGT Version: v2.2.4
Faiss Version: v1.8.0

Checklist

I have read the CONTRIBUTING document and completed our CLA agreement.
I have checked open Pull Requests for the similar feature or fixes?

Special notes for your reviewer

Summary by CodeRabbit

New Features
- Introduced new gRPC metadata definitions and documentation.
- Expanded Kubernetes configurations for agent and discoverer components.
- Added a new Rust module for handling metadata.
Bug Fixes
- Updated imagePullPolicy settings for various containers to ensure the latest images are always pulled.
Version Updates
- Incremented version numbers for various dependencies, including cloud libraries and AWS SDKs, to the latest releases.
- Updated software versions for Chaos Mesh (2.7.0), Docker (v27.3.1), Helm (v3.16.1), KUBECTL (v1.31.1), Prometheus stack (63.1.0), Protobuf (28.2), Reviewdog (v0.20.2), and several GitHub Actions (including ACTIONS_CHECKOUT from 4.1.7 to 4.2.0).
- Updated several GitHub Actions to version 2.19.0 for the CodeQL actions and increased the version for GITHUB_ISSUE_METRICS to 3.12.0.
- Incremented version for PETER_EVANS_CREATE_ISSUE_FROM_FILE to 5.0.1 and PETER_EVANS_CREATE_PULL_REQUEST to 7.0.5.

coderabbitai · 2024-09-21T05:50:45Z

📝 Walkthrough

<details>
<summary>📝 Walkthrough</summary>

## Walkthrough

This pull request introduces substantial modifications across various components, including the addition of new protocol buffer definitions, updates to Kubernetes configurations, and enhancements to Rust source files. Key changes involve new gRPC metadata definitions, error handling improvements, adjustments to image pull policies in Kubernetes manifests, and the establishment of a new Rust module for metadata handling. The overall changes reflect an expansion of functionality and structural updates aimed at improving the codebase's organization and maintainability.

## Changes

| File(s)                                                                 | Change Summary                                                                                                           |
|-------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------|
| `.gitfiles`, `apis/grpc/v1/meta/meta.pb.go`, `apis/grpc/v1/meta/meta_vtproto.pb.go`, `apis/proto/v1/meta/meta.proto`, `apis/swagger/v1/meta/meta.swagger.json`, `internal/core/algorithm/option.go`, `internal/core/algorithm/usearch.go`, `internal/core/algorithm/usearch_test.go`, `internal/errors/usearch.go`, `rust/bin/meta/Cargo.toml`, `rust/bin/meta/handler.rs`, `rust/bin/meta/handler/meta.rs`, `rust/bin/meta/main.rs`, `rust/libs/proto/meta.v1.tonic.rs`, `versions/USEARCH_VERSION` | Introduction of new gRPC metadata definitions and Rust source files, along with a new version variable for `usearch`. |
| `charts/vald/values.yaml`                                               | Addition of `imagePullPolicy: Always` for multiple `wait-for` containers in the `gateway` and `manager` sections.     |
| `k8s/gateway/gateway/lb/deployment.yaml`, `k8s/index/job/correction/cronjob.yaml` | Updates to `imagePullPolicy` settings for `wait-for` containers, setting them to `Always`.                            |
| `k8s/index/operator/configmap.yaml`                                     | Updates to `imagePullPolicy` settings for specific containers, changing from `Always` to `IfNotPresent`.                |
| `versions/*`                                                            | Various version updates across multiple dependencies and components, reflecting new releases and enhancements.           |

## Possibly related PRs

- **#2611**: This PR addresses broken links in documentation and includes updates to the `Makefile.d/k8s.mk`, which is relevant to the changes in the main PR that also involve Kubernetes configurations.
- **#2642**: This PR modifies the ingress route settings in `ing.yaml`, which is directly related to the changes in the main PR that enhance routing capabilities and introduce new configurations for gRPC services.

## Suggested labels

`type/feature`, `priority/medium`, `size/XXXL`

</details>

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

cloudflare-workers-and-pages · 2024-09-21T05:53:23Z

Deploying vald with Cloudflare Pages

Latest commit:	`9ff43ed`
Status:	✅ Deploy successful!
Preview URL:	https://6b874b68.vald.pages.dev
Branch Preview URL:	https://refactor-main-for-release-v1.vald.pages.dev

View logs

codecov · 2024-09-21T06:17:52Z

Codecov Report

Attention: Patch coverage is 18.11594% with 452 lines in your changes missing coverage. Please review.

Project coverage is 24.30%. Comparing base (0039af1) to head (0a14610).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/net/grpc/status/status.go	24.36%	287 Missing and 11 partials ⚠️
internal/net/grpc/errdetails/errdetails.go	0.00%	76 Missing ⚠️
internal/net/grpc/codes/codes.go	0.00%	38 Missing ⚠️
...l/net/grpc/interceptor/server/logging/accesslog.go	0.00%	20 Missing ⚠️
internal/net/dialer.go	14.28%	11 Missing and 1 partial ⚠️
internal/net/grpc/pool/pool.go	0.00%	4 Missing ⚠️
hack/docker/gen/main.go	0.00%	2 Missing ⚠️
internal/net/grpc/client.go	0.00%	1 Missing ⚠️
pkg/gateway/lb/handler/grpc/handler.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2639      +/-   ##
==========================================
- Coverage   24.36%   24.30%   -0.07%     
==========================================
  Files         536      537       +1     
  Lines       46459    46950     +491     
==========================================
+ Hits        11320    11410      +90     
- Misses      34372    34759     +387     
- Partials      767      781      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

coderabbitai

Actionable comments posted: 15

Outside diff range and nitpick comments (9)

k8s/tools/cli/loadtest/cronjob.yaml (1)

40-40: Optimize resource usage by avoiding unnecessary image pulls.

Changing imagePullPolicy from Always to IfNotPresent can reduce unnecessary load on the image registry and speed up container starts if the image already exists on the node.

Ensure that the image tag (nightly in this case) is carefully managed to avoid running unintended versions of the image when imagePullPolicy is set to IfNotPresent. Consider using unique tags for each version or a specific semantic version tag instead of nightly.
docs/user-guides/filtering-configuration.md (1)
156-156: Nit: Replace hard tab with spaces.

The static analysis tool flagged the use of a hard tab for indentation on this line. To maintain consistency with the surrounding code and improve readability, consider replacing the hard tab with spaces.
-       conn, err := grpc.NewClient(grpcServerAddr, grpc.WithInsecure())
+    conn, err := grpc.NewClient(grpcServerAddr, grpc.WithInsecure())
Tools

Markdownlint

156-156: Column: 1
Hard tabs

(MD010, no-hard-tabs)
docs/user-guides/client-api-config.md (1)

49-49: Suggestion: Replace hard tabs with spaces.

Markdownlint has detected the presence of hard tabs in the modified lines. To improve the consistency and readability of the markdown file, consider replacing the hard tabs with spaces.

Also applies to: 165-165, 289-289, 473-473, 656-656

Tools

Markdownlint

49-49: Column: 1
Hard tabs

(MD010, no-hard-tabs)
k8s/index/operator/configmap.yaml (2)
Use specific version tags instead of floating tags.

The verification process has revealed an issue with image tag management:

In k8s/index/operator/configmap.yaml, the image tag is set to "nightly", which is a floating tag.

Using floating tags like "nightly" can lead to inconsistent deployments, especially with imagePullPolicy: IfNotPresent.

Recommendations:

Replace the "nightly" tag with a specific version tag in the main configuration file.

Ensure all production deployments use specific version tags for better consistency and easier rollback if needed.

Consider implementing a CI/CD process that automatically updates the image tags to the latest stable version during the release process.

Analysis chain

Line range hint 280-280: Verify image tag management.

The imagePullPolicy change to IfNotPresent can speed up deployments by skipping unnecessary image pulls. However, please ensure that the image tag is being properly managed to avoid running unintended versions.

Run the following script to verify the image tag:
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify that the image tag is not using a floating tag like `latest`.

# Test: Search for the image tag. Expect: No floating tags like `latest` or `nightly`.
rg --type yaml $'image: \"vdaas/vald-readreplica-rotate'
Length of output: 243

Script:
#!/bin/bash
# Description: Search for the vald-readreplica-rotate image with a flexible pattern

# Search for any yaml file containing "vald-readreplica-rotate" in the image field
rg --type yaml 'image:.*vald-readreplica-rotate'

# If the above doesn't yield results, search for any "readreplica" or "rotate" in the image field
rg --type yaml 'image:.*read.*replica|image:.*rotate'
Length of output: 51222
Verify image tag management and standardize across files.

The imagePullPolicy change to IfNotPresent is present in the configmap.yaml file as intended. However, there are some concerns regarding image tag management:

The "nightly" tag is used in some files (e.g., k8s/index/job/save/cronjob.yaml), which is not recommended for production environments as it's a floating tag.

There's an inconsistency in image tags across different files:

Some use "nightly"

Others use "pr-2444"

To ensure proper version control and consistency, please consider the following:

Standardize the image tags across all files to use specific version tags instead of "nightly".

Use the same specific version tag (e.g., "pr-2444" or a semantic version) for all vald-index-* images to maintain consistency.

Update the following files to use consistent, specific version tags:

k8s/index/job/save/cronjob.yaml

k8s/index/operator/configmap.yaml

k8s/index/job/creation/cronjob.yaml

k8s/index/job/correction/cronjob.yaml

This will help avoid potential version mismatches and improve the predictability of deployments.

Analysis chain

Line range hint 754-754: Verify image tag management.

The imagePullPolicy change to IfNotPresent can speed up deployments by skipping unnecessary image pulls. However, please ensure that the image tag is being properly managed to avoid running unintended versions.

Run the following script to verify the image tag:
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify that the image tag is not using a floating tag like `latest`.

# Test: Search for the image tag. Expect: No floating tags like `latest` or `nightly`.
rg --type yaml $'image: \"vdaas/vald-index-save' 
Length of output: 243

Script:
#!/bin/bash
# Description: Search for vald-index-save or similar image names in YAML files

# Search for vald-index-save
echo "Searching for vald-index-save:"
rg --type yaml 'image:.*vald-index-save'

# Search for vald-index with surrounding context
echo -e "\nSearching for vald-index with context:"
rg --type yaml -C 2 'image:.*vald-index'
Length of output: 54001
internal/net/grpc/client.go (1)

168-168: Clarify the rationale behind removing the grpc.WithBlock() option.

Removing the blocking behavior on the g.Connect call could lead to scenarios where the connection is not ready when accessed later. While the existing error handling should mitigate this, it's important to consider:

How does this change align with the overall design goals?

Is the non-blocking behavior desirable in all scenarios?

Have you thoroughly tested this change to ensure it doesn't introduce unexpected connection issues?

Please provide more context on the motivation behind this change.
internal/net/grpc/status/status.go (2)
Line range hint 243-426: Refactor 'withDetails' function to reduce code duplication

The withDetails function contains repetitive patterns when aggregating error details for various types. Similar logic is used across multiple switch cases, leading to lengthy and hard-to-maintain code.

Consider extracting common behaviors into helper functions. For example, you can create a generic function to handle the aggregation of messages, reducing redundancy and improving readability.

717-749: Handle unknown message types explicitly in 'typeURL' function

Currently, the typeURL function returns "unknown" for unhandled proto.Message types without any logging or error handling.

Consider logging a warning when an unknown type is encountered. This will aid in debugging and ensure that new message types are registered appropriately.

Apply this diff to add logging:
 func typeURL(msg proto.Message) string {
     switch msg.(type) {
     // [existing cases]
+    default:
+        log.Warnf("typeURL: unrecognized message type %T", msg)
+        return "unknown"
     }
-    return "unknown"
 }
.gitfiles (1)

1034-1034: Consistent Error Handling in usearch.go

The error definitions in internal/errors/usearch.go should follow the project's conventions for error handling. This ensures consistency and makes error management easier across the codebase.

Consider using error wrapping and predefined error variables where appropriate.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between bcebbdc and 3bea900.

Files ignored due to path filters (6)

apis/grpc/v1/agent/sidecar/sidecar_vtproto.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
apis/grpc/v1/payload/payload.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
apis/grpc/v1/rpc/errdetails/error_details.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
example/client/go.sum is excluded by !**/*.sum
go.sum is excluded by !**/*.sum
rust/Cargo.lock is excluded by !**/*.lock

Files selected for processing (65)

.gitfiles (9 hunks)
.github/ISSUE_TEMPLATE/bug_report.md (1 hunks)
.github/ISSUE_TEMPLATE/security_issue_report.md (1 hunks)
.github/PULL_REQUEST_TEMPLATE.md (3 hunks)
Makefile (2 hunks)
charts/vald/templates/_helpers.tpl (1 hunks)
charts/vald/values.yaml (7 hunks)
cmd/index/operator/sample.yaml (4 hunks)
dockers/agent/core/agent/Dockerfile (1 hunks)
docs/tutorial/get-started-with-faiss-agent.md (1 hunks)
docs/tutorial/get-started.md (1 hunks)
docs/tutorial/vald-agent-standalone-on-k8s.md (1 hunks)
docs/user-guides/client-api-config.md (5 hunks)
docs/user-guides/filtering-configuration.md (1 hunks)
example/client/agent/main.go (1 hunks)
example/client/go.mod (2 hunks)
example/client/main.go (1 hunks)
example/client/mirror/main.go (1 hunks)
example/manifest/scylla/job.yaml (1 hunks)
go.mod (11 hunks)
hack/docker/gen/main.go (1 hunks)
internal/net/grpc/client.go (1 hunks)
internal/net/grpc/errdetails/errdetails.go (2 hunks)
internal/net/grpc/interceptor/server/logging/accesslog.go (5 hunks)
internal/net/grpc/pool/pool.go (4 hunks)
internal/net/grpc/pool/pool_bench_test.go (2 hunks)
internal/net/grpc/status/status.go (4 hunks)
internal/observability/exporter/otlp/otlp.go (1 hunks)
internal/observability/trace/status.go (1 hunks)
k8s/agent/statefulset.yaml (1 hunks)
k8s/discoverer/deployment.yaml (1 hunks)
k8s/external/minio/deployment.yaml (1 hunks)
k8s/external/minio/mb-job.yaml (1 hunks)
k8s/gateway/gateway/lb/deployment.yaml (3 hunks)
k8s/gateway/gateway/mirror/deployment.yaml (2 hunks)
k8s/index/job/correction/cronjob.yaml (3 hunks)
k8s/index/job/creation/cronjob.yaml (3 hunks)
k8s/index/job/save/cronjob.yaml (3 hunks)
k8s/index/operator/configmap.yaml (1 hunks)
k8s/index/operator/deployment.yaml (2 hunks)
k8s/manager/index/deployment.yaml (3 hunks)
k8s/operator/helm/operator.yaml (1 hunks)
k8s/tools/benchmark/operator/deployment.yaml (1 hunks)
k8s/tools/cli/loadtest/cronjob.yaml (1 hunks)
k8s/tools/cli/loadtest/job.yaml (1 hunks)
tests/e2e/operation/multi.go (8 hunks)
tests/e2e/operation/operation.go (4 hunks)
tests/e2e/operation/stream.go (13 hunks)
tests/e2e/performance/max_vector_dim_test.go (2 hunks)
versions/CHAOS_MESH_VERSION (1 hunks)
versions/DOCKER_VERSION (1 hunks)
versions/HELM_VERSION (1 hunks)
versions/K3S_VERSION (1 hunks)
versions/KUBECTL_VERSION (1 hunks)
versions/OPERATOR_SDK_VERSION (1 hunks)
versions/PROMETHEUS_STACK_VERSION (1 hunks)
versions/PROTOBUF_VERSION (1 hunks)
versions/REVIEWDOG_VERSION (1 hunks)
versions/actions/ACTIONS_SETUP_NODE (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_ANALYZE (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_INIT (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF (1 hunks)
versions/actions/GITHUB_ISSUE_METRICS (1 hunks)
versions/actions/PETER_EVANS_CREATE_PULL_REQUEST (1 hunks)

Files skipped from review due to trivial changes (7)

.github/PULL_REQUEST_TEMPLATE.md
versions/HELM_VERSION
versions/KUBECTL_VERSION
versions/PROTOBUF_VERSION
versions/REVIEWDOG_VERSION
versions/actions/ACTIONS_SETUP_NODE
versions/actions/PETER_EVANS_CREATE_PULL_REQUEST

Additional context used

Learnings (2)

docs/tutorial/vald-agent-standalone-on-k8s.md (1)

Learnt from: kpango
PR: vdaas/vald#2638
File: docs/tutorial/vald-agent-standalone-on-k8s.md:239-239
Timestamp: 2024-09-21T05:35:36.818Z
Learning: In grpc-go, `grpc.DialContext` has been deprecated and `grpc.NewClient` should be used instead.

example/client/agent/main.go (1)

Learnt from: kpango
PR: vdaas/vald#2638
File: docs/tutorial/vald-agent-standalone-on-k8s.md:239-239
Timestamp: 2024-09-21T05:35:36.818Z
Learning: In grpc-go, `grpc.DialContext` has been deprecated and `grpc.NewClient` should be used instead.

Markdownlint

docs/user-guides/client-api-config.md

49-49: Column: 1
Hard tabs

(MD010, no-hard-tabs)

165-165: Column: 1
Hard tabs

(MD010, no-hard-tabs)

289-289: Column: 1
Hard tabs

(MD010, no-hard-tabs)

473-473: Column: 1
Hard tabs

(MD010, no-hard-tabs)

656-656: Column: 1
Hard tabs

(MD010, no-hard-tabs)

docs/user-guides/filtering-configuration.md

156-156: Column: 1
Hard tabs

(MD010, no-hard-tabs)

Additional comments not posted (116)

versions/CHAOS_MESH_VERSION (1)

1-1: Verify the impact and compatibility of the version update.

The version update itself looks good. However, please ensure that:

The new version 2.7.0 has been thoroughly tested and validated.

The updated version is compatible with the rest of the system and does not introduce any breaking changes.

Any relevant documentation, such as release notes or upgrade guides, has been reviewed and updated if necessary.
versions/OPERATOR_SDK_VERSION (1)
1-1: Verify the impact of the SDK version update on the project.

The version update from v1.33 to v1.37 suggests there have been enhancements, bug fixes, or new features introduced in the SDK. However, the specific details of what changes occurred in functionality, logic, or control flow are not provided in the diff.

Please ensure that:

The project's dependencies are compatible with the new SDK version.

The version update does not introduce any breaking changes that might affect the project's functionality.

Run the following script to verify the SDK version usage across the project:

Verification successful

SDK version update verified and properly integrated

The version update from v1.33 to v1.37 in the versions/OPERATOR_SDK_VERSION file has been verified. This change is correctly reflected in the project, and the OPERATOR_SDK_VERSION is referenced in various build and configuration files, ensuring proper integration throughout the project.

Key findings:

The new version v1.37 is present in the versions/OPERATOR_SDK_VERSION file.

Multiple files reference OPERATOR_SDK_VERSION, indicating it's used in the build process.

No occurrences of the old version (v1.33) were found, suggesting a comprehensive update.

The update appears to be isolated to the versions file, with other files dynamically referencing this file rather than hardcoding the version. This approach allows for easier future updates and maintains consistency across the project.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the SDK version usage across the project.

# Test: Search for the SDK version usage. Expect: Only occurrences of the new version.
rg --type-add 'k8s:*.yaml' -t k8s $'OPERATOR_SDK_VERSION'
Length of output: 56

Script:
#!/bin/bash
# Description: Verify the SDK version usage and project structure

# Test 1: Check for YAML files in the project
echo "Checking for YAML files:"
fd -e yaml -e yml

# Test 2: Search for OPERATOR_SDK_VERSION in all files
echo -e "\nSearching for OPERATOR_SDK_VERSION in all files:"
rg "OPERATOR_SDK_VERSION"

# Test 3: Search for the new version number
echo -e "\nSearching for v1.37 in all files:"
rg "v1.37"

# Test 4: Verify if ripgrep is working
echo -e "\nVerifying ripgrep functionality:"
rg "README" README.md
Length of output: 13722
versions/PROMETHEUS_STACK_VERSION (1)

1-1: Approve the version update.

The version update from 62.6.0 to 62.7.0 aligns with the PR objective of updating various dependencies.

Please provide additional information about the changes introduced in this version, such as new features, bug fixes, or performance improvements. This will help ensure that the version update is compatible with the rest of the codebase and doesn't introduce any breaking changes.

versions/actions/GITHUB_CODEQL_ACTION_ANALYZE (1)

1-1: Version update noted.

The version number for GITHUB_CODEQL_ACTION_ANALYZE has been incremented from 2.18.4 to 2.19.0. This suggests there may be new features, bug fixes, or improvements in the newer version.

If there are any specific changes or reasons for updating to this version that are relevant to the PR objectives, please provide more context. Otherwise, the version update itself looks fine.

versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD (1)

1-1: Verify compatibility and changes in the new version.

Updating the version number is a good practice to ensure the project is using the latest version of the dependency. The new version may include bug fixes, security patches, or new features that could be beneficial to the project.

However, it's important to verify that the new version is compatible with the project and does not introduce any breaking changes. Please review the changelog or release notes of the new version to understand the specific changes and assess their impact on the project.

versions/actions/GITHUB_CODEQL_ACTION_INIT (1)

1-1: Version increment looks good.

The version number has been incremented from 2.18.4 to 2.19.0, which aligns with common versioning practices to indicate modifications or improvements in the software.

versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF (1)

1-1: Verify compatibility and impact of the version update.

Updating to the latest version is generally a good practice to ensure you have the latest features, performance improvements, and bug fixes.

However, before merging this change, please verify that:

The new version 2.19.0 is compatible with your current setup and doesn't introduce any breaking changes.

The update doesn't negatively impact your CI/CD pipeline or any dependent workflows.

It's recommended to review the release notes or changelog of the GITHUB_CODEQL_ACTION_UPLOAD_SARIF action to understand the specific changes introduced in this version and assess if they align with your requirements.

versions/actions/GITHUB_ISSUE_METRICS (1)

1-1: Version update looks good!

The minor version bump from 3.9.0 to 3.11.0 suggests the introduction of new backward-compatible functionality or bug fixes. Updating the version number is important for proper tracking and differentiation between releases.

versions/DOCKER_VERSION (1)

1-1: Approved: Docker version updated to v27.3.1.

The Docker version has been updated to v27.3.1, which follows semantic versioning. Based on the version increment, this update likely includes new functionality and bug fixes that are backwards-compatible.

However, to better understand the impact of this update on the codebase, it would be helpful to have more information about the specific changes included in this version.

Could you please provide a summary of the notable changes, features, or bug fixes introduced in Docker version v27.3.1? This will help ensure that our codebase remains compatible and can leverage any relevant improvements.

versions/K3S_VERSION (1)

1-1: Verify compatibility and impact of the K3S version update.

The version update from v1.30.2-k3s2 to v1.31.1-k3s1 is approved. However, please ensure that:

The new version is compatible with the rest of the system and doesn't introduce any breaking changes.

The specific enhancements, bug fixes, or modifications in the new version are reviewed and their impact on the overall functionality is assessed.

.github/ISSUE_TEMPLATE/security_issue_report.md (1)

22-24: LGTM!

The version updates for Docker, Kubernetes, and Helm are minor increments and align with the PR objective of updating dependencies. The changes do not introduce any new logic or alter the structure of the template.

.github/ISSUE_TEMPLATE/bug_report.md (1)

28-30: LGTM!

The version updates for Docker, Kubernetes, and Helm are consistent with the PR objectives and the AI-generated summary. The updates are minor and unlikely to introduce any breaking changes. They are likely to be beneficial in terms of bug fixes, performance improvements, and security patches.

k8s/external/minio/mb-job.yaml (1)

26-26: Approve the imagePullPolicy change with a note about manual updates.

Changing the imagePullPolicy to IfNotPresent can speed up pod startup times by avoiding unnecessary pulls of the image if it already exists on the node. This is a good efficiency improvement.

However, please ensure you have a process to manually update the image when needed, as updates to the image in the registry won't be automatically pulled with this policy. The old version will continue to be used until the image is manually pulled or the node is replaced.

example/manifest/scylla/job.yaml (1)

26-26: LGTM, but be mindful of updating the image tag.

Changing imagePullPolicy to IfNotPresent can speed up pod startup times and reduce load on the image registry, which is a positive change.

However, please ensure that you update the image tag (e.g., cassandra:latest) whenever a new version of the scylla-init image is released. Otherwise, the job may run using an outdated version of the image.

k8s/external/minio/deployment.yaml (1)

36-36: LGTM! Ensure a controlled process for updating the image version when needed.

Changing imagePullPolicy from Always to IfNotPresent can improve pod startup performance by avoiding unnecessary image pulls when the required version is already cached on the node.

However, this means pods may not always run the latest image version, as they can reuse an older cached version. Ensure there is a controlled process to update the image tag and roll out the new version when needed, such as by updating the deployment template and triggering a rolling update.

k8s/tools/cli/loadtest/job.yaml (1)

36-36: Approve the imagePullPolicy change with considerations.

Changing the imagePullPolicy from Always to IfNotPresent can help reduce unnecessary image pulls and improve startup time when the image is already cached on the node.

However, please consider the following trade-offs:

If the vald-loadtest image is expected to change frequently, using IfNotPresent may result in running stale or inconsistent versions across nodes.

If consistent image versions across all replicas are critical, Always might be more appropriate to ensure the latest image is pulled every time.

Evaluate these factors in the context of your deployment strategy and image update frequency to determine the most suitable imagePullPolicy.

example/client/go.mod (3)

17-17: LGTM!

The minor version update of google.golang.org/grpc from v1.66.2 to v1.67.0 is appropriate and aligns with the PR objective of updating dependencies.

36-36: LGTM!

The minor version update of golang.org/x/net from v0.26.0 to v0.28.0 is appropriate and aligns with the PR objective of updating dependencies.

39-39: LGTM!

The patch version update of google.golang.org/genproto/googleapis/api from v0.0.0-20240604185151-ef581f913117 to v0.0.0-20240814211410-ddb44dafa142 is appropriate and aligns with the PR objective of updating dependencies.

dockers/agent/core/agent/Dockerfile (1)

43-43: LGTM!

The reordering of the CARGO_HOME environment variable declaration above the RUSTUP_HOME declaration improves the readability and maintainability of the Dockerfile by grouping related environment variables together. The change does not affect the functionality or correctness of the Dockerfile.

k8s/operator/helm/operator.yaml (1)

46-46: Verify if the IfNotPresent policy aligns with the deployment strategy.

The change from Always to IfNotPresent for the imagePullPolicy can help optimize the deployment process by reducing unnecessary image pulls. This can result in faster pod startup times and reduced load on the image registry.

However, please ensure that the IfNotPresent policy aligns with your overall deployment strategy and CI/CD workflow. If you frequently update the vald-helm-operator image, you might still need the Always policy to ensure the latest image is pulled.

You can verify this by reviewing your CI/CD pipeline configuration and deployment processes.

k8s/tools/benchmark/operator/deployment.yaml (1)

46-46: LGTM!

The change from Always to IfNotPresent for the imagePullPolicy can have the following impact:

Pros:

Reduces the number of image pulls, which can speed up pod startup times and reduce network traffic.

Cons:

If the image is updated in the registry, the pods will not automatically use the new version until they are restarted.

Consider these trade-offs and ensure that this change aligns with your deployment strategy.

internal/net/grpc/pool/pool_bench_test.go (2)

132-132: LGTM!

The change from grpc.DialContext to grpc.NewClient aligns with the gRPC documentation's recommendation and should not have any significant impact on the benchmark's functionality or performance.

189-189: LGTM!

Similar to the change in Benchmark_StaticDial, the transition from grpc.DialContext to grpc.NewClient in this benchmark function is in line with the gRPC documentation's recommendation. It should not have any significant impact on the benchmark's functionality or performance.

internal/observability/exporter/otlp/otlp.go (1)

34-34: Looks good! Keeping dependencies updated is a good practice.

Updating the OpenTelemetry semantic conventions package to version v1.26.0 ensures that the latest features, bug fixes, and improvements are leveraged in the codebase. This aligns with the best practice of keeping dependencies up to date.

internal/observability/trace/status.go (1)

24-24: LGTM! The version update aligns with the best practice of keeping dependencies up to date.

Please double-check that the semantic convention attribute keys used in this file (semconv.RPCGRPCStatusCode*) are still valid and have the same meaning in version v1.26.0 of the package.

k8s/index/job/save/cronjob.yaml (3)

56-56: LGTM!

Setting imagePullPolicy to IfNotPresent for the wait-for-agent init container is a good optimization. It will reduce unnecessary image pulls when the image already exists on the node.

68-68: Looks good!

Changing the imagePullPolicy to IfNotPresent for the wait-for-discoverer init container is a sensible optimization. It will prevent unnecessary image pulls when the image is already present on the node.

81-81: Approved, but be mindful of the image tag.

Changing imagePullPolicy from Always to IfNotPresent for the vald-index-save container is a good optimization to reduce unnecessary image pulls and speed up container startup.

However, please ensure that the nightly image tag is updated whenever a new version of the vald-index-save image needs to be deployed. With imagePullPolicy set to IfNotPresent, Kubernetes will continue using the old image if the tag remains unchanged.

k8s/index/job/creation/cronjob.yaml (3)

56-56: LGTM!

Setting imagePullPolicy: IfNotPresent for the wait-for-agent initContainer is a good optimization. It will avoid unnecessary image pulls when the image is already present on the node.

68-68: LGTM!

Setting imagePullPolicy: IfNotPresent for the wait-for-discoverer initContainer is a good optimization. It will avoid unnecessary image pulls when the image is already present on the node.

81-81: Verify that the desired image version is pre-pulled on the nodes.

Changing imagePullPolicy from Always to IfNotPresent for the main vald-index-creation container can help optimize resource usage by avoiding unnecessary image pulls on every pod creation.

However, please ensure that the desired image version is already present on the nodes. If not, the pods may end up using an outdated version, potentially leading to unexpected behavior or compatibility issues.

k8s/index/job/correction/cronjob.yaml (1)

56-56: LGTM!

Setting imagePullPolicy to IfNotPresent for the wait-for-agent initContainer can help optimize pod startup time by avoiding unnecessary image pulls when the image is already cached on the node.

Just keep in mind that if the image tag changes in the future, it won't be automatically updated on nodes that already have it cached. So make sure to use stable, specific image tags with this policy.

k8s/index/operator/deployment.yaml (1)

49-49: LGTM!

The checksum update reflects changes in the associated configmap and will trigger a deployment update. This is the expected behavior to keep the deployment in sync with configmap changes.

k8s/discoverer/deployment.yaml (1)

80-80: Consider the implications of changing imagePullPolicy to IfNotPresent.

Changing imagePullPolicy from Always to IfNotPresent can improve efficiency by avoiding unnecessary image pulls when the image already exists on the node. However, keep in mind that with this policy, nodes will not automatically pull updates to the image when a new version is pushed to the registry with the same tag. The pod would need to be restarted to pull the latest version.

Ensure this behavior aligns with your desired container update strategy.

docs/user-guides/filtering-configuration.md (1)

156-156: LGTM! Ensure all relevant documentation and examples are updated.

The change from grpc.DialContext to grpc.NewClient for establishing the gRPC client connection looks good. It aligns with the overall refactoring objective of the pull request.

Please make sure to update any other documentation or examples that may still reference the old grpc.DialContext method to maintain consistency across the codebase.

Tools

Markdownlint

156-156: Column: 1
Hard tabs

(MD010, no-hard-tabs)

tests/e2e/performance/max_vector_dim_test.go (1)

Line range hint 128-156: LGTM! The changes align with gRPC best practices.

The transition from grpc.DialContext to grpc.NewClient for establishing the gRPC client connection is in line with the gRPC documentation and best practices. This update ensures compatibility and adherence to the recommended approach.

Additionally, the movement of the context variable declaration to after the client connection establishment is a minor refactoring that does not impact the overall functionality.

The error handling and subsequent operations remain unchanged, maintaining the existing logic.

k8s/gateway/gateway/mirror/deployment.yaml (2)

58-58: Consider the tradeoffs of using IfNotPresent for the init container.

The change to use IfNotPresent as the imagePullPolicy for the wait-for-gateway-lb init container can speed up deployments by avoiding unnecessary image pulls. However, it's important to consider the tradeoff that this may cause the use of stale or outdated images in some scenarios.

Ensure that you have processes in place to regularly update the base image to mitigate this risk.

89-89: Ensure processes are in place to manage image updates with IfNotPresent policy.

Changing the imagePullPolicy from Always to IfNotPresent for the vald-mirror-gateway container is a good optimization to speed up deployments by avoiding unnecessary image pulls.

However, it's crucial to have robust processes to ensure the image is updated when needed, such as when there are bug fixes or new features. This will prevent running outdated or vulnerable versions of the image for extended periods.

Consider implementing automated processes to check for and pull new versions of the image on a regular cadence.

tests/e2e/operation/operation.go (5)

Line range hint 153-161: LGTM!

The changes to the CreateIndex function are consistent with the updated getAgentClient() method signature. The function logic remains correct.

Line range hint 166-174: LGTM!

The changes to the SaveIndex function are consistent with the updated getAgentClient() method signature. The function logic remains correct.

177-183: LGTM!

The changes to the IndexInfo function are consistent with the updated getClient() method signature. The function logic remains correct.

199-206: LGTM!

The changes to the getClient function are consistent with the updated getGRPCConn() method signature. The function logic remains correct.

Line range hint 208-214: LGTM!

The changes to the getAgentClient function are consistent with the updated getGRPCConn() method signature. The function logic remains correct.

k8s/manager/index/deployment.yaml (2)

62-62: Consider the tradeoffs of using IfNotPresent for the init container.

Setting imagePullPolicy to IfNotPresent can speed up pod startup time by avoiding unnecessary image pulls. However, keep in mind that if the image is updated, it won't be automatically pulled unless the pod is recreated. Ensure this aligns with your desired update strategy.

74-74: LGTM!

The imagePullPolicy change for the wait-for-discoverer init container is consistent with the wait-for-agent init container. The same considerations mentioned in the previous comment apply here.

k8s/gateway/gateway/lb/deployment.yaml (3)

61-61: LGTM!

Setting imagePullPolicy to IfNotPresent for the wait-for-discoverer init container is a good optimization. It will avoid unnecessary image pulls if the image already exists on the node.

73-73: Looks good!

Changing the imagePullPolicy to IfNotPresent for the wait-for-agent init container is a sensible optimization. It will prevent unnecessary image pulls when the image is already available on the node.

104-104: Verify if this change aligns with the desired behavior.

Changing the imagePullPolicy from Always to IfNotPresent for the main vald-lb-gateway container can help reduce unnecessary image pulls and speed up deployments. However, please consider the following:

If the image tag remains static (e.g., nightly) but the underlying image changes, the new image won't be pulled automatically. This could lead to inconsistencies between pods.

If the desired behavior is to always use the latest nightly image, then Always would be more appropriate.

Please confirm if IfNotPresent aligns with the intended behavior. If not, consider reverting to Always.

tests/e2e/operation/multi.go (2)

63-63: Verify if getClient has been updated to no longer require the context parameter.

Similar to the MultiSearch method, there seems to be an inconsistency in how the context parameter ctx is used:

It has been removed from the getClient call, suggesting that getClient no longer requires it.

However, ctx is still being passed to the MultiSearchByID method call, indicating that it is required for the gRPC call.

Please ensure that the removal of ctx from getClient aligns with its current signature and usage. If getClient still expects the context parameter, this change could lead to compile errors or unexpected behavior.

99-99: Verify if getClient has been updated to no longer require the context parameter.

As with the previous methods, there seems to be an inconsistency in how the context parameter ctx is used:

It has been removed from the getClient call, suggesting that getClient no longer requires it.

However, ctx is still being passed to the MultiLinearSearch method call, indicating that it is required for the gRPC call.

Please ensure that the removal of ctx from getClient aligns with its current signature and usage. If getClient still expects the context parameter, this change could lead to compile errors or unexpected behavior.

example/client/main.go (1)

69-69: LGTM! The change aligns with the PR objective and gRPC best practices.

The transition from grpc.DialContext to grpc.NewClient for establishing the gRPC connection is in line with the PR objective and follows the recommended approach as per gRPC documentation. The parameters passed to grpc.NewClient are consistent with the previous implementation, ensuring no functional differences or breakages are introduced.

example/client/mirror/main.go (1)

71-71: Approve the gRPC connection establishment refactoring, but request more context.

The change from grpc.DialContext to grpc.NewClient for establishing the gRPC connection seems to be an intentional refactoring. However, grpc.NewClient does not appear to be a standard gRPC function.

To better understand the implications of this change, could you please provide more context about the grpc.NewClient function? Is it a custom wrapper around grpc.DialContext? What additional functionality or configuration does it provide?

Understanding the behavior of grpc.NewClient will help ensure that this change does not introduce any unintended side effects in how the gRPC connection is established and managed.

example/client/agent/main.go (1)

70-70: LGTM!

The change from grpc.DialContext to grpc.NewClient is consistent with the gRPC documentation, which indicates that grpc.DialContext has been deprecated in favor of grpc.NewClient.

The parameters passed to grpc.NewClient and the error handling logic remain unchanged, ensuring a smooth transition to the new method.

The learning provided by kpango in PR #2638 also confirms this change:

In grpc-go, grpc.DialContext has been deprecated and grpc.NewClient should be used instead.

internal/net/grpc/errdetails/errdetails.go (1)

61-74: LGTM!

Exporting the error detail message name constants is a good change that improves the usability of the package.

Note that this is a breaking change for any code that was previously using the unexported names.
docs/tutorial/get-started-with-faiss-agent.md (1)
281-281: Verify all occurrences of grpc.DialContext have been updated and thoroughly test the change.

The change from grpc.DialContext to grpc.NewClient aligns with the gRPC documentation and the PR objective of removing the deprecated method. Please ensure:

All occurrences of grpc.DialContext across the codebase have been consistently updated to grpc.NewClient.

Thorough testing is performed to validate the behavior of grpc.NewClient in all relevant scenarios.

Run the following script to verify grpc.DialContext is no longer used in the codebase:

If the above test passes with no results found, it confirms grpc.DialContext is no longer used in the codebase.

Verification successful

Verification successful: grpc.DialContext has been removed from the codebase

The change from grpc.DialContext to grpc.NewClient has been successfully implemented across the codebase. Multiple search methods confirmed that grpc.DialContext is no longer present in any Go files.

The removal of grpc.DialContext is consistent throughout the project.

The change aligns with the PR objective of removing the deprecated method.

Remember to thoroughly test the new grpc.NewClient implementation to ensure it functions correctly in all scenarios.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify `grpc.DialContext` is no longer used in the codebase.

# Test: Search for `grpc.DialContext` usage. Expect: No results found.
rg --type go $'grpc\.DialContext'
Length of output: 34

Script:
#!/bin/bash
# Description: Perform additional checks to verify the absence of `grpc.DialContext` and validate the search functionality.

# Test 1: Verify rg is working by searching for a common Go keyword
echo "Test 1: Searching for 'package' keyword"
rg --type go 'package'

# Test 2: Use grep as an alternative search method
echo "Test 2: Using grep to search for grpc.DialContext"
grep -R --include="*.go" "grpc\.DialContext" .

# Test 3: Check for the presence of Go files
echo "Test 3: Checking for Go files"
find . -name "*.go" | head -n 5

# Test 4: Double-check for grpc.DialContext using find and grep
echo "Test 4: Using find and grep to search for grpc.DialContext"
find . -name "*.go" -exec grep -H "grpc\.DialContext" {} +
Length of output: 72839
docs/tutorial/vald-agent-standalone-on-k8s.md (1)

239-239: LGTM!

The change from grpc.DialContext to grpc.NewClient aligns with the grpc-go recommendation, as grpc.DialContext has been deprecated.

This is consistent with the learning:

In grpc-go, grpc.DialContext has been deprecated and grpc.NewClient should be used instead.
docs/tutorial/get-started.md (1)
336-336: Verify the impact of changing the gRPC connection method.

The change from grpc.DialContext to grpc.NewClient for establishing the gRPC connection is approved. However, please verify the following:

Ensure that this change aligns with the latest gRPC best practices and recommendations.

Confirm that changing the connection method does not introduce any unexpected behavior or performance implications.

Verify that this change is consistently applied across the codebase wherever gRPC connections are established.

To verify the impact of this change, run the following script:

Verification successful

Verification successful: gRPC connection method change is consistently applied

The change from grpc.DialContext to grpc.NewClient has been consistently implemented across the codebase. This systematic update suggests an intentional shift in the gRPC client creation process, likely aligning with updated best practices or internal standards. Key observations:

No instances of the old grpc.DialContext method were found.

grpc.NewClient is used consistently in various parts of the codebase, including examples, tests, and internal implementations.

The usage pattern is consistent, typically including the server address and appropriate credentials.

Recommendations:

Ensure that the rationale for this change is documented, especially regarding any performance or behavior differences.

Update any relevant documentation or developer guides to reflect this new connection method.

Consider adding a comment in the code explaining the reason for using grpc.NewClient over grpc.DialContext for future maintainers.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of grpc.NewClient across the codebase.

# Test 1: Search for usage of the old connection method grpc.DialContext. Expect: No results.
rg --type go $'grpc\.DialContext'

# Test 2: Search for usage of the new connection method grpc.NewClient. Expect: Only occurrences with the same parameter signature as the reviewed code.
rg --type go --context 3 $'grpc\.NewClient'
Length of output: 5294
docs/user-guides/client-api-config.md (5)

49-49: LGTM!

The change from grpc.DialContext to grpc.NewClient is consistent with the refactoring objective to transition to the updated gRPC client creation method as per the gRPC documentation.

Tools

Markdownlint

49-49: Column: 1
Hard tabs

(MD010, no-hard-tabs)

165-165: LGTM!

Similar to the previous comment, the change from grpc.DialContext to grpc.NewClient is consistent with the refactoring objective.

Tools

Markdownlint

165-165: Column: 1
Hard tabs

(MD010, no-hard-tabs)

289-289: LGTM!

Once again, the change from grpc.DialContext to grpc.NewClient is consistent with the refactoring objective mentioned in the previous comments.

Tools

Markdownlint

289-289: Column: 1
Hard tabs

(MD010, no-hard-tabs)

473-473: LGTM!

The change from grpc.DialContext to grpc.NewClient is consistent with the refactoring objective, as mentioned in the previous comments.

Tools

Markdownlint

473-473: Column: 1
Hard tabs

(MD010, no-hard-tabs)

656-656: LGTM!

As with the previous instances, the change from grpc.DialContext to grpc.NewClient is consistent with the refactoring objective.

Tools

Markdownlint

656-656: Column: 1
Hard tabs

(MD010, no-hard-tabs)

internal/net/grpc/pool/pool.go (3)

Line range hint 130-146: LGTM!

The refactor from grpc.DialContext to grpc.NewClient aligns with the gRPC documentation and is a straightforward change. The error handling and connection establishment flow remain consistent.

Line range hint 472-492: LGTM!

The refactor from grpc.DialContext to grpc.NewClient aligns with the gRPC documentation and is a straightforward change. The updated error handling logic using errors.Join improves the error reporting by combining multiple errors, if any.

Line range hint 700-709: LGTM!

The refactor from grpc.DialContext to grpc.NewClient aligns with the gRPC documentation and is a straightforward change. The port scanning logic and error handling remain consistent.

cmd/index/operator/sample.yaml (4)

202-202: Ensure image tags are updated when publishing new versions.

The imagePullPolicy change from Always to IfNotPresent aligns with best practices and can improve pod startup time. However, please be cautious to always update the image tag when publishing a new version of the vald-readreplica-rotate component. Failing to do so risks running stale images since IfNotPresent only pulls the image if it doesn't already exist on the node.

338-338: Ensure image tags are updated when publishing new versions.

The imagePullPolicy change from Always to IfNotPresent aligns with best practices and can improve job execution time. However, please be cautious to always update the image tag when publishing a new version of the vald-index-creation component. Failing to do so risks running stale images since IfNotPresent only pulls the image if it doesn't already exist on the node.

453-453: Ensure image tags are updated when publishing new versions.

The imagePullPolicy change from Always to IfNotPresent aligns with best practices and can improve job execution time. However, please be cautious to always update the image tag when publishing a new version of the vald-index-save component. Failing to do so risks running stale images since IfNotPresent only pulls the image if it doesn't already exist on the node.

568-568: Ensure image tags are updated when publishing new versions.

The imagePullPolicy change from Always to IfNotPresent aligns with best practices and can improve job execution time. However, please be cautious to always update the image tag when publishing a new version of the vald-index-correction component. Failing to do so risks running stale images since IfNotPresent only pulls the image if it doesn't already exist on the node.
tests/e2e/operation/stream.go (13)
296-296: See previous comment.

The same verification applies here as the context parameter has been removed from the getClient method call.

416-416: See previous comments.

The same verification applies here as the context parameter has been removed from the getClient method call.

534-534: See previous comments.

The same verification applies here as the context parameter has been removed from the getClient method call.

640-640: See previous comments.

The same verification applies here as the context parameter has been removed from the getClient method call.

743-743: See previous comments.

The same verification applies here as the context parameter has been removed from the getClient method call.

847-847: See previous comments.

The same verification applies here as the context parameter has been removed from the getClient method call.

949-949: See previous comments.

The same verification applies here as the context parameter has been removed from the getClient method call.

1032-1032: See previous comments.

The same verification applies here as the context parameter has been removed from the getClient method call.

1050-1050: See previous comments.

The same verification applies here as the context parameter has been removed from the getClient method call.

1077-1077: See previous comments.

The same verification applies here as the context parameter has been removed from the getClient method call.

1099-1099: See previous comments.

The same verification applies here as the context parameter has been removed from the getClient method call.

1189-1189: See previous comments.

The same verification applies here as the context parameter has been removed from the getClient method call.

96-96: Verify the removal of the context parameter.

The ctx parameter has been removed from the getClient method call. Ensure that this change is intended and that the client calls are not relying on the context for cancellation or deadlines.

To verify the impact of removing the context parameter, run the following script:

Verification successful

To gather more information about the getClient method and its potential use of context, let's run the following script:

Let's try a more comprehensive search to locate the getClient method and its usage:

Removal of context parameter from getClient method is consistent and likely safe.

The removal of the ctx parameter from the getClient method call appears to be an intentional change that is consistent across the tests/e2e/operation/stream.go file. This modification aligns with the method's implementation in tests/e2e/operation/operation.go, where getClient is defined without a context parameter.

While the context is no longer passed to getClient, it's important to note that:

The context is still being used in the subsequent client method calls (e.g., client.StreamSearch(ctx)).

This change simplifies the getClient method without compromising the context-aware operations in the actual client methods.

Given these observations, the removal of the context parameter from the getClient method call is likely safe and does not impact the cancellation or deadline propagation behavior of the client calls.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify if client calls are using context for cancellation or deadlines.

# Test: Search for `WithDeadline`, `WithTimeout` or `WithCancel` calls. 
# Expect: No occurrences if context is not being used for cancellation or deadlines.
rg --type go -A 5 $'With(Deadline|Timeout|Cancel)'
Length of output: 140093

Script:
#!/bin/bash
# Search for the getClient method implementation and any context usage within it
ast-grep --lang go --pattern 'func $_ getClient($_) $_ {
  $$$
}'

# Search for any context usage within the getClient method or related functions
rg --type go -A 10 'func (\w+\.)?getClient'
Length of output: 113

Script:
#!/bin/bash
# Search for getClient or GetClient method definitions
echo "Searching for getClient/GetClient method definitions:"
rg --type go '(func|method) (\w+\.)?(get|Get)[cC]lient\b' -A 5

# Search for struct or interface definitions that include getClient or GetClient
echo "\nSearching for struct/interface definitions with getClient/GetClient:"
rg --type go '(type \w+ struct|type \w+ interface).*\n.*(\b(get|Get)[cC]lient\b)' -A 10

# Search for calls to getClient or GetClient
echo "\nSearching for calls to getClient/GetClient:"
rg --type go '\b(\w+\.)?(get|Get)[cC]lient\b\(' -A 3

# Specifically search in the tests/e2e/operation/stream.go file
echo "\nSearching in tests/e2e/operation/stream.go:"
rg --type go '(get|Get)[cC]lient' tests/e2e/operation/stream.go -A 5
Length of output: 21262
hack/docker/gen/main.go (1)

417-417: LGTM!

The change adds an additional check to prevent duplicate entries when merging values for the same key across multiple maps. This helps in avoiding redundant values in the concatenated string.
k8s/index/operator/configmap.yaml (3)
28-28: LGTM!

The configuration changes look good and enhance the performance, observability, and resource management aspects of the Kubernetes operator.

Line range hint 991-991: Verify image tag management.

The imagePullPolicy change to IfNotPresent can speed up deployments by skipping unnecessary image pulls. However, please ensure that the image tag is being properly managed to avoid running unintended versions.

Run the following script to verify the image tag:
#!/bin/bash
# Description: Verify that the image tag is not using a floating tag like `latest`.  

# Test: Search for the image tag. Expect: No floating tags like `latest` or `nightly`.
rg --type yaml $'image: \"vdaas/vald-index-correction'
Line range hint 517-517: Verify image tag management.

The imagePullPolicy change to IfNotPresent can speed up deployments by skipping unnecessary image pulls. However, please ensure that the image tag is being properly managed to avoid running unintended versions.

Run the following script to verify the image tag:
#!/bin/bash 
# Description: Verify that the image tag is not using a floating tag like `latest`.

# Test: Search for the image tag. Expect: No floating tags like `latest` or `nightly`.  
rg --type yaml $'image: \"vdaas/vald-index-creation'
go.mod (16)

7-7: LGTM!

The minor version update for cloud.google.com/go/bigquery from v1.62.0 to v1.63.0 looks good. It should be a safe update that brings improvements without introducing breaking changes.

8-8: LGTM!

The patch version update for cloud.google.com/go/compute from v1.28.0 to v1.28.1 looks good. It should only include backwards-compatible bug fixes and is safe to update.

11-11: LGTM!

The patch version update for cloud.google.com/go/iam from v1.2.0 to v1.2.1 looks good. It should only include backwards-compatible bug fixes and is safe to update.

12-12: LGTM!

The minor version update for cloud.google.com/go/kms from v1.19.0 to v1.20.0 looks good. It should be a safe update that brings improvements without introducing breaking changes.

13-13: LGTM!

The patch version update for cloud.google.com/go/monitoring from v1.21.0 to v1.21.1 looks good. It should only include backwards-compatible bug fixes and is safe to update.

15-15: LGTM!

The patch version update for cloud.google.com/go/secretmanager from v1.14.0 to v1.14.1 looks good. It should only include backwards-compatible bug fixes and is safe to update.

17-17: LGTM!

The patch version update for cloud.google.com/go/trace from v1.11.0 to v1.11.1 looks good. It should only include backwards-compatible bug fixes and is safe to update.

41-41: Please provide more context on the specific commit.

The dependency github.com/ajstarks/deck has been updated to a pseudo-version v0.0.0-20240918141114-8d365813662d based on a specific commit hash.

Could you please provide more information about the changes introduced in this specific commit and the reason for locking the dependency to a commit hash instead of using a release version? This will help assess the impact of this update on the codebase.

42-42: Synced version with main module.

The dependency github.com/ajstarks/deck/generate has been updated to the same pseudo-version v0.0.0-20240918141114-8d365813662d as the main github.com/ajstarks/deck module.

Keeping the versions in sync between the main module and its submodules is a good practice.

48-48: Minor version update looks good, but review changelog.

The dependency github.com/aws/aws-sdk-go-v2 has been updated from v1.30.5 to v1.31.0, which is a minor version update.

This update may introduce new features, enhancements, bug fixes, and performance improvements. The changes should be backwards compatible, but it's recommended to review the changelog to understand the new changes and their potential impact on the codebase.

49-49: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream from v1.6.4 to v1.6.5 looks good. It should only include backwards-compatible bug fixes and is safe to update.

50-50: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/config from v1.27.33 to v1.27.36 looks good. It should only include backwards-compatible bug fixes and is safe to update.

51-51: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/credentials from v1.17.32 to v1.17.34 looks good. It should only include backwards-compatible bug fixes and is safe to update.

52-52: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/feature/ec2/imds from v1.16.13 to v1.16.14 looks good. It should only include backwards-compatible bug fixes and is safe to update.

53-53: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/feature/s3/manager from v1.17.18 to v1.17.22 looks good. It should only include backwards-compatible bug fixes and is safe to update.

54-54: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/internal/configsources from v1.3.17 to v1.3.18 looks good. It should only include backwards-compatible bug fixes and is safe to update.

charts/vald/templates/_helpers.tpl (1)

702-702: LGTM!

The new imagePullPolicy attribute allows for dynamic configuration of the init container's image pull behavior based on the provided context. This enhances flexibility in controlling when the image is fetched from the repository.

charts/vald/values.yaml (6)

1078-1078: LGTM!

Setting the imagePullPolicy to IfNotPresent for the wait-for-discoverer initContainer is a good practice. It avoids unnecessary image pulls and can speed up pod startup times.

1084-1084: LGTM!

Setting the imagePullPolicy to IfNotPresent for the wait-for-agent initContainer is a good practice. It avoids unnecessary image pulls and can speed up pod startup times.

1362-1362: LGTM!

Setting the imagePullPolicy to IfNotPresent for the wait-for-gateway-lb initContainer is a good practice. It avoids unnecessary image pulls and can speed up pod startup times.

1650-1650: LGTM!

Setting the imagePullPolicy to IfNotPresent for the wait-for-gateway-lb initContainer is a good practice. It avoids unnecessary image pulls and can speed up pod startup times.

3140-3140: LGTM!

Setting the imagePullPolicy to IfNotPresent for the wait-for-agent initContainer is a good practice. It avoids unnecessary image pulls and can speed up pod startup times.

3146-3146: LGTM!

Setting the imagePullPolicy to IfNotPresent for the wait-for-discoverer initContainer is a good practice. It avoids unnecessary image pulls and can speed up pod startup times.
internal/net/grpc/status/status.go (1)
21-21: Verify compatibility with Go versions due to use of 'cmp' and 'slices' packages

The imports "cmp" and "slices" are part of the Go standard library starting from Go 1.21. If the project aims to maintain compatibility with earlier Go versions, this could introduce build issues.

Run the following script to check the Go version specified in the go.mod file:

Also applies to: 24-24

Verification successful

Go version compatibility verified for 'cmp' and 'slices' packages

The project is using Go version 1.23.1 as specified in the go.mod file. This version is newer than Go 1.21, which introduced the 'cmp' and 'slices' packages to the standard library. Therefore, there are no compatibility issues with using these packages in the project.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check the Go version used in the project.

# Test: Display the Go version specified in go.mod. Expect: go 1.21 or later.
cat go.mod | grep '^go '
Length of output: 36
Makefile (1)

668-668: Formatting Consistency Confirmed

The indentation and formatting of -DFAISS_ENABLE_GPU=OFF \ align with the surrounding code.
.gitfiles (4)
2045-2045: Update USEARCH Version Definition

The versions/USEARCH_VERSION file has been added. Ensure that the version specified aligns with the version used in the codebase and that any version upgrades are tested for compatibility.

Confirm that the version specified works correctly with your implementation.

1412-1423: Review Kubernetes ClusterRole and ServiceAccount Permissions

The Kubernetes configuration files for the agent (k8s/agent/clusterrole.yaml, clusterrolebinding.yaml, serviceaccount.yaml) should grant only the minimum necessary permissions following the principle of least privilege.

Please verify that the roles and bindings are appropriately scoped. You can check the permissions using:
#!/bin/bash
# Description: Verify ClusterRole permissions for the agent.

kubectl describe clusterrole <agent-clusterrole-name>
1976-1993: Ensure Protobuf Bindings in Rust Are Up-to-Date

The Rust protobuf bindings such as core.v1.rs, discoverer.v1.rs, filter.egress.v1.rs, filter.ingress.v1.rs, meta.v1.rs, mirror.v1.rs, sidecar.v1.rs, and vald.v1.rs should be regenerated to match any changes in the .proto files.

Run the following script to regenerate the Rust protobuf files:
#!/bin/bash
# Description: Regenerate Rust protobuf bindings.

PROTO_DIR="apis/proto/v1"
OUT_DIR="rust/libs/proto/src"

protoc -I "$PROTO_DIR" \
  --rust_out="$OUT_DIR" \
  "$PROTO_DIR"/*.proto
226-226: Ensure Swagger Documentation is Up-to-Date

The Swagger specification apis/swagger/v1/meta/meta.swagger.json should accurately reflect the meta.proto definitions. Changes in the .proto file should trigger a regeneration of the Swagger JSON to keep API documentation consistent.

Run the following script to regenerate the Swagger file and compare it with the committed version:

If differences are found, please update the Swagger file accordingly.

k8s/index/job/correction/cronjob.yaml

k8s/agent/statefulset.yaml

coderabbitai · 2024-09-21T06:33:58Z

k8s/index/operator/deployment.yaml

@@ -74,7 +74,7 @@ spec:
      containers:
        - name: vald-index-operator
          image: "vdaas/vald-index-operator:nightly"
-          imagePullPolicy: Always
+          imagePullPolicy: IfNotPresent


Tip
Codebase Verification

Consider using a specific version tag instead of "nightly"

The image tag for the vald-index-operator container is currently set to "nightly". This, combined with imagePullPolicy: IfNotPresent, may lead to inconsistencies across environments:

The "nightly" tag typically refers to the latest development build, which can change frequently.

With IfNotPresent policy, different nodes might use different versions of the "nightly" build, depending on when they last pulled the image.

To ensure consistency and reproducibility across all environments, consider:

Using a specific version tag (e.g., v1.2.3) instead of "nightly".

If using "nightly" is necessary for development purposes, consider keeping imagePullPolicy: Always to ensure the latest nightly build is always used.

Analysis chain

Verify that a specific image version is used.

Changing imagePullPolicy to IfNotPresent can reduce unnecessary image pulls and improve pod startup times. However, please ensure that the vald-index-operator container specifies a version tag other than latest to maintain consistency across environments.

Run the following script to verify the image tag:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash # Description: Verify that the vald-index-operator container uses a specific image version. # Test: Search for the image definition. Expect: A version tag other than 'latest' or 'nightly'. rg --type yaml $'image:\s*"vdaas/vald-index-operator' k8s/index/operator/deployment.yaml

Length of output: 142

coderabbitai · 2024-09-21T06:33:58Z

k8s/manager/index/deployment.yaml

@@ -91,7 +93,7 @@ spec:
      containers:
        - name: vald-manager-index
          image: "vdaas/vald-manager-index:nightly"
-          imagePullPolicy: Always
+          imagePullPolicy: IfNotPresent


Ensure the IfNotPresent pull policy is compatible with your update strategy.

Changing imagePullPolicy from Always to IfNotPresent for the main container can speed up pod restarts. However, with the RollingUpdate deployment strategy, this could lead to inconsistencies during updates, as pods on nodes with older cached versions will not pull the new image.

Consider one of the following:

Stick with imagePullPolicy: Always for the main container to ensure all pods are running the same version after an update.

If using IfNotPresent, ensure you have a strategy to manually force pods to restart after an image update (e.g., by changing a deployment annotation).

Use an alternative mechanism to ensure pods are restarted with the new image, such as tagging images with a unique identifier (e.g., a hash of the image contents) and updating that tag in the deployment spec.

internal/net/grpc/status/status.go

Makefile

.gitfiles

coderabbitai

Actionable comments posted: 10

Outside diff range and nitpick comments (15)

example/client/go.mod (2)
36-36: Consider updating the version of golang.org/x/net to match the replace directive.

The version of golang.org/x/net used in the file (v0.28.0) is older than the one specified in the replace directive (v0.29.0). While this is generally safe, as the replace directive takes precedence, it's a good practice to keep the versions consistent to avoid confusion and potential issues in the future.

Consider applying this diff to update the version:
 require (
 	buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.34.2-20240717164558-a6c49f84cc0f.2 // indirect
 	github.com/goccy/go-json v0.10.2 // indirect
 	github.com/kpango/fastime v1.1.9 // indirect
 	github.com/planetscale/vtprotobuf v0.6.1-0.20240319094008-0393e58bdf10 // indirect
-	golang.org/x/net v0.28.0 // indirect
+	golang.org/x/net v0.29.0 // indirect
 	golang.org/x/sys v0.25.0 // indirect
 	golang.org/x/text v0.18.0 // indirect
 	google.golang.org/genproto/googleapis/api v0.0.0-20240814211410-ddb44dafa142 // indirect
39-39: Consider updating the version of google.golang.org/genproto/googleapis/api to match the replace directive.

The version of google.golang.org/genproto/googleapis/api used in the file (v0.0.0-20240814211410-ddb44dafa142) is older than the one specified in the replace directive (v0.0.0-20240903143218-8af14fe29dc1). While this is generally safe, as the replace directive takes precedence, it's a good practice to keep the versions consistent to avoid confusion and potential issues in the future.

Consider applying this diff to update the version:
 	golang.org/x/net v0.28.0 // indirect
 	golang.org/x/sys v0.25.0 // indirect
 	golang.org/x/text v0.18.0 // indirect
-	google.golang.org/genproto/googleapis/api v0.0.0-20240814211410-ddb44dafa142 // indirect
+	google.golang.org/genproto/googleapis/api v0.0.0-20240903143218-8af14fe29dc1 // indirect
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20240827150818-7e3bb234dfed // indirect
 	google.golang.org/protobuf v1.34.2 // indirect
 )
k8s/index/job/creation/cronjob.yaml (1)

56-56: Consider the tradeoffs of using IfNotPresent for the init container image pull policy.

Setting imagePullPolicy to IfNotPresent can speed up pod startup time by avoiding unnecessary image pulls if the image already exists locally. However, it's important to consider:

Any updates to the busybox:stable image in the registry won't be automatically pulled.

If this image is frequently updated with important fixes, you might miss out on those by using IfNotPresent.

Evaluate if the speedup is worth potentially running an outdated init container image and adjust the policy if needed.

k8s/agent/statefulset.yaml (1)

79-79: Approve, but consider using a specific image version tag.

The change of imagePullPolicy from Always to IfNotPresent can help improve pod startup times by avoiding unnecessary image pulls when the image already exists locally.

However, since the vald-agent-ngt image is using the nightly tag, there's a risk of inconsistencies between the versions of images running on different pods over time. As a best practice, consider using a specific image version tag instead of nightly. This ensures all pods are running the exact same version of the code.
docs/user-guides/filtering-configuration.md (1)
156-156: Replace hard tab with spaces.

To improve the portability and consistency of the code example, consider replacing the hard tab at the beginning of line 156 with spaces for indentation.
-	conn, err := grpc.NewClient(grpcServerAddr, grpc.WithInsecure())
+   conn, err := grpc.NewClient(grpcServerAddr, grpc.WithInsecure())
Tools

Markdownlint

156-156: Column: 1
Hard tabs

(MD010, no-hard-tabs)
internal/net/grpc/pool/pool.go (4)

130-130: Verify test coverage for the grpc.NewClient usage.

The change from grpc.DialContext to grpc.NewClient looks good and aligns with the PR objective.

However, the static analysis tool indicates that this line is not covered by tests. Please ensure adequate test coverage for this change.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools

GitHub Check: codecov/patch

[warning] 130-130: internal/net/grpc/pool/pool.go#L130
Added line #L130 was not covered by tests

146-146: Verify test coverage for the grpc.NewClient usage.

The change from grpc.DialContext to grpc.NewClient looks good and aligns with the PR objective.

However, the static analysis tool indicates that this line is not covered by tests. Please ensure adequate test coverage for this change.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools

GitHub Check: codecov/patch

[warning] 146-146: internal/net/grpc/pool/pool.go#L146
Added line #L146 was not covered by tests

472-472: Verify test coverage for the grpc.NewClient usage.

The change from grpc.DialContext to grpc.NewClient looks good and aligns with the PR objective.

However, the static analysis tool indicates that this line is not covered by tests. Please ensure adequate test coverage for this change.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools

GitHub Check: codecov/patch

[warning] 472-472: internal/net/grpc/pool/pool.go#L472
Added line #L472 was not covered by tests

700-700: Verify test coverage for the grpc.NewClient usage.

The change from grpc.DialContext to grpc.NewClient looks good and aligns with the PR objective.

However, the static analysis tool indicates that this line is not covered by tests. Please ensure adequate test coverage for this change.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools

GitHub Check: codecov/patch

[warning] 700-700: internal/net/grpc/pool/pool.go#L700
Added line #L700 was not covered by tests

hack/docker/gen/main.go (1)

417-417: Add a test case to cover the changed line.

The optimization to avoid duplicating values in the concatenated string looks good. However, the static analysis tool indicates that the added line is not covered by tests.

To mitigate the risk of introducing bugs, please add a test case that specifically covers the scenario where the value from the current map (v) already contains the existing value from the result map (ev).

Do you want me to generate the missing test case or open a GitHub issue to track this task?

Tools

GitHub Check: codecov/patch

[warning] 417-417: hack/docker/gen/main.go#L417
Added line #L417 was not covered by tests

internal/net/grpc/interceptor/server/logging/accesslog.go (1)

118-118: Verify the appropriateness of logging levels for RPC failures.

Currently, failed RPC calls are logged using log.Warn. Depending on the importance of these logs and the logging strategy, it might be more appropriate to use log.Error to highlight failures that need attention.

Review the logging levels to ensure they align with the desired verbosity and monitoring requirements.

Also applies to: 166-166

Tools

GitHub Check: codecov/patch

[warning] 118-118: internal/net/grpc/interceptor/server/logging/accesslog.go#L118
Added line #L118 was not covered by tests

internal/net/grpc/status/status.go (2)

Line range hint 243-426: Complexity in withDetails Function May Affect Maintainability

The withDetails function (lines 243-426) contains extensive type assertions and repetitive code patterns for aggregating error details. This complexity can make the code harder to maintain and prone to bugs.

Consider refactoring the function to reduce repetition and improve readability. One possible approach is to create a helper function to handle the aggregation logic for each error detail type. This can reduce code duplication and simplify future updates.

Tools

GitHub Check: codecov/patch

[warning] 272-281: internal/net/grpc/status/status.go#L272-L281
Added lines #L272 - L281 were not covered by tests

[warning] 288-297: internal/net/grpc/status/status.go#L288-L297
Added lines #L288 - L297 were not covered by tests

702-709: Simplify Error Message Handling in withDetails Function

In lines 702-709, the error message is constructed based on whether err is nil. The current logic may result in an empty or less informative error message if both err and st.Message() are empty.

Consider defaulting to a generic message or ensuring that errMsg always contains meaningful information to improve error traceability.
.gitfiles (2)
208-208: Add Documentation to the New meta.proto File

The new meta.proto file lacks comments for messages and services. Adding documentation will improve maintainability and help other developers understand the API.

Apply this diff to add comments:
syntax = "proto3";

package apis.proto.v1.meta;

+// MetaService provides metadata operations.
service MetaService {
  // Define RPC methods here
}
2045-2045: Validate USEARCH_VERSION Integration

Confirm that the USEARCH_VERSION is correctly incorporated into the build and release processes. Ensure the version is documented and that dependency management handles this new addition appropriately.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between bcebbdc and 3bea900.

Files ignored due to path filters (6)

apis/grpc/v1/agent/sidecar/sidecar_vtproto.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
apis/grpc/v1/payload/payload.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
apis/grpc/v1/rpc/errdetails/error_details.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
example/client/go.sum is excluded by !**/*.sum
go.sum is excluded by !**/*.sum
rust/Cargo.lock is excluded by !**/*.lock

Files selected for processing (65)

.gitfiles (9 hunks)
.github/ISSUE_TEMPLATE/bug_report.md (1 hunks)
.github/ISSUE_TEMPLATE/security_issue_report.md (1 hunks)
.github/PULL_REQUEST_TEMPLATE.md (3 hunks)
Makefile (2 hunks)
charts/vald/templates/_helpers.tpl (1 hunks)
charts/vald/values.yaml (7 hunks)
cmd/index/operator/sample.yaml (4 hunks)
dockers/agent/core/agent/Dockerfile (1 hunks)
docs/tutorial/get-started-with-faiss-agent.md (1 hunks)
docs/tutorial/get-started.md (1 hunks)
docs/tutorial/vald-agent-standalone-on-k8s.md (1 hunks)
docs/user-guides/client-api-config.md (5 hunks)
docs/user-guides/filtering-configuration.md (1 hunks)
example/client/agent/main.go (1 hunks)
example/client/go.mod (2 hunks)
example/client/main.go (1 hunks)
example/client/mirror/main.go (1 hunks)
example/manifest/scylla/job.yaml (1 hunks)
go.mod (11 hunks)
hack/docker/gen/main.go (1 hunks)
internal/net/grpc/client.go (1 hunks)
internal/net/grpc/errdetails/errdetails.go (2 hunks)
internal/net/grpc/interceptor/server/logging/accesslog.go (5 hunks)
internal/net/grpc/pool/pool.go (4 hunks)
internal/net/grpc/pool/pool_bench_test.go (2 hunks)
internal/net/grpc/status/status.go (4 hunks)
internal/observability/exporter/otlp/otlp.go (1 hunks)
internal/observability/trace/status.go (1 hunks)
k8s/agent/statefulset.yaml (1 hunks)
k8s/discoverer/deployment.yaml (1 hunks)
k8s/external/minio/deployment.yaml (1 hunks)
k8s/external/minio/mb-job.yaml (1 hunks)
k8s/gateway/gateway/lb/deployment.yaml (3 hunks)
k8s/gateway/gateway/mirror/deployment.yaml (2 hunks)
k8s/index/job/correction/cronjob.yaml (3 hunks)
k8s/index/job/creation/cronjob.yaml (3 hunks)
k8s/index/job/save/cronjob.yaml (3 hunks)
k8s/index/operator/configmap.yaml (1 hunks)
k8s/index/operator/deployment.yaml (2 hunks)
k8s/manager/index/deployment.yaml (3 hunks)
k8s/operator/helm/operator.yaml (1 hunks)
k8s/tools/benchmark/operator/deployment.yaml (1 hunks)
k8s/tools/cli/loadtest/cronjob.yaml (1 hunks)
k8s/tools/cli/loadtest/job.yaml (1 hunks)
tests/e2e/operation/multi.go (8 hunks)
tests/e2e/operation/operation.go (4 hunks)
tests/e2e/operation/stream.go (13 hunks)
tests/e2e/performance/max_vector_dim_test.go (2 hunks)
versions/CHAOS_MESH_VERSION (1 hunks)
versions/DOCKER_VERSION (1 hunks)
versions/HELM_VERSION (1 hunks)
versions/K3S_VERSION (1 hunks)
versions/KUBECTL_VERSION (1 hunks)
versions/OPERATOR_SDK_VERSION (1 hunks)
versions/PROMETHEUS_STACK_VERSION (1 hunks)
versions/PROTOBUF_VERSION (1 hunks)
versions/REVIEWDOG_VERSION (1 hunks)
versions/actions/ACTIONS_SETUP_NODE (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_ANALYZE (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_INIT (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF (1 hunks)
versions/actions/GITHUB_ISSUE_METRICS (1 hunks)
versions/actions/PETER_EVANS_CREATE_PULL_REQUEST (1 hunks)

Files skipped from review due to trivial changes (9)

.github/PULL_REQUEST_TEMPLATE.md
versions/DOCKER_VERSION
versions/HELM_VERSION
versions/KUBECTL_VERSION
versions/PROTOBUF_VERSION
versions/REVIEWDOG_VERSION
versions/actions/ACTIONS_SETUP_NODE
versions/actions/GITHUB_CODEQL_ACTION_INIT
versions/actions/PETER_EVANS_CREATE_PULL_REQUEST

Additional context used

Learnings (2)

docs/tutorial/vald-agent-standalone-on-k8s.md (1)

Learnt from: kpango
PR: vdaas/vald#2638
File: docs/tutorial/vald-agent-standalone-on-k8s.md:239-239
Timestamp: 2024-09-21T05:35:36.818Z
Learning: In grpc-go, `grpc.DialContext` has been deprecated and `grpc.NewClient` should be used instead.

example/client/agent/main.go (1)

Learnt from: kpango
PR: vdaas/vald#2638
File: docs/tutorial/vald-agent-standalone-on-k8s.md:239-239
Timestamp: 2024-09-21T05:35:36.818Z
Learning: In grpc-go, `grpc.DialContext` has been deprecated and `grpc.NewClient` should be used instead.

Markdownlint

docs/user-guides/client-api-config.md

49-49: Column: 1
Hard tabs

(MD010, no-hard-tabs)

165-165: Column: 1
Hard tabs

(MD010, no-hard-tabs)

289-289: Column: 1
Hard tabs

(MD010, no-hard-tabs)

473-473: Column: 1
Hard tabs

(MD010, no-hard-tabs)

656-656: Column: 1
Hard tabs

(MD010, no-hard-tabs)

docs/user-guides/filtering-configuration.md

156-156: Column: 1
Hard tabs

(MD010, no-hard-tabs)

GitHub Check: codecov/patch

hack/docker/gen/main.go

[warning] 417-417: hack/docker/gen/main.go#L417
Added line #L417 was not covered by tests

internal/net/grpc/client.go

[warning] 168-168: internal/net/grpc/client.go#L168
Added line #L168 was not covered by tests

internal/net/grpc/errdetails/errdetails.go

[warning] 237-238: internal/net/grpc/errdetails/errdetails.go#L237-L238
Added lines #L237 - L238 were not covered by tests

[warning] 244-244: internal/net/grpc/errdetails/errdetails.go#L244
Added line #L244 was not covered by tests

[warning] 250-250: internal/net/grpc/errdetails/errdetails.go#L250
Added line #L250 was not covered by tests

[warning] 256-256: internal/net/grpc/errdetails/errdetails.go#L256
Added line #L256 was not covered by tests

[warning] 262-262: internal/net/grpc/errdetails/errdetails.go#L262
Added line #L262 was not covered by tests

[warning] 268-268: internal/net/grpc/errdetails/errdetails.go#L268
Added line #L268 was not covered by tests

[warning] 274-274: internal/net/grpc/errdetails/errdetails.go#L274
Added line #L274 was not covered by tests

[warning] 280-280: internal/net/grpc/errdetails/errdetails.go#L280
Added line #L280 was not covered by tests

[warning] 286-286: internal/net/grpc/errdetails/errdetails.go#L286
Added line #L286 was not covered by tests

[warning] 292-292: internal/net/grpc/errdetails/errdetails.go#L292
Added line #L292 was not covered by tests

[warning] 298-298: internal/net/grpc/errdetails/errdetails.go#L298
Added line #L298 was not covered by tests

[warning] 304-304: internal/net/grpc/errdetails/errdetails.go#L304
Added line #L304 was not covered by tests

[warning] 310-310: internal/net/grpc/errdetails/errdetails.go#L310
Added line #L310 was not covered by tests

[warning] 316-316: internal/net/grpc/errdetails/errdetails.go#L316
Added line #L316 was not covered by tests

internal/net/grpc/interceptor/server/logging/accesslog.go

[warning] 56-63: internal/net/grpc/interceptor/server/logging/accesslog.go#L56-L63
Added lines #L56 - L63 were not covered by tests

[warning] 66-70: internal/net/grpc/interceptor/server/logging/accesslog.go#L66-L70
Added lines #L66 - L70 were not covered by tests

[warning] 73-74: internal/net/grpc/interceptor/server/logging/accesslog.go#L73-L74
Added lines #L73 - L74 were not covered by tests

[warning] 76-76: internal/net/grpc/interceptor/server/logging/accesslog.go#L76
Added line #L76 was not covered by tests

[warning] 118-118: internal/net/grpc/interceptor/server/logging/accesslog.go#L118
Added line #L118 was not covered by tests

[warning] 120-120: internal/net/grpc/interceptor/server/logging/accesslog.go#L120
Added line #L120 was not covered by tests

[warning] 166-166: internal/net/grpc/interceptor/server/logging/accesslog.go#L166
Added line #L166 was not covered by tests

[warning] 168-168: internal/net/grpc/interceptor/server/logging/accesslog.go#L168
Added line #L168 was not covered by tests

internal/net/grpc/pool/pool.go

[warning] 130-130: internal/net/grpc/pool/pool.go#L130
Added line #L130 was not covered by tests

[warning] 146-146: internal/net/grpc/pool/pool.go#L146
Added line #L146 was not covered by tests

[warning] 472-472: internal/net/grpc/pool/pool.go#L472
Added line #L472 was not covered by tests

[warning] 700-700: internal/net/grpc/pool/pool.go#L700
Added line #L700 was not covered by tests

internal/net/grpc/status/status.go

[warning] 272-281: internal/net/grpc/status/status.go#L272-L281
Added lines #L272 - L281 were not covered by tests

[warning] 288-297: internal/net/grpc/status/status.go#L288-L297
Added lines #L288 - L297 were not covered by tests

Additional comments not posted (97)

versions/CHAOS_MESH_VERSION (1)

1-1: Version update looks good.

The version has been updated from 2.6.4 to 2.7.0, indicating a minor version update. Minor version updates are expected to introduce new features, enhancements, or bug fixes while maintaining backward compatibility.

versions/OPERATOR_SDK_VERSION (1)

1-1: Approve the Operator SDK version update.

Updating the Operator SDK version from v1.33 to v1.37 is a good practice to ensure access to the latest features, bug fixes, and performance improvements.

To better understand the impact of this update on the project, please provide more information about the specific changes introduced in the Operator SDK between versions v1.33 and v1.37. This will help ensure a smooth transition and identify any potential compatibility issues or required modifications in the codebase.

versions/PROMETHEUS_STACK_VERSION (1)

1-1: Version update: Review release notes and perform testing.

The version of the Prometheus stack has been updated from 62.6.0 to 62.7.0. This indicates a minor version bump, which typically introduces new features, improvements, or bug fixes while maintaining backward compatibility.

Please review the release notes or changelog associated with this version to understand the specific changes and their implications. Additionally, ensure that thorough testing is performed to verify that the version update does not introduce any regressions or compatibility issues in the codebase.
versions/actions/GITHUB_CODEQL_ACTION_ANALYZE (1)
1-1: Verify compatibility of the updated version.

The minor version update from 2.18.4 to 2.19.0 is unlikely to introduce breaking changes. However, please ensure that the updated version is compatible with the rest of the codebase and dependencies.

Run the following script to verify compatibility:

If the tests pass, then the updated version is likely compatible with the codebase and dependencies. If there are any discrepancies, please investigate further.

Verification successful

CodeQL action update is compatible with current usage.

The update from version 2.18.4 to 2.19.0 in the versions/actions/GITHUB_CODEQL_ACTION_ANALYZE file is compatible with the current usage in the project's workflows. The CodeQL action is referenced using the major version v2 in the codeql-analysis.yaml workflow, which allows for automatic updates to the latest minor and patch versions. No changes to the workflows are necessary, and the update will be automatically applied in future workflow runs.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify compatibility of the updated version with the codebase and dependencies.

# Test 1: Search for any references to the specific version `2.18.4` in the codebase.
# Expect: No occurrences of the old version.
rg --type-not markdown --type-not txt $'2\.18\.4'

# Test 2: Search for any references to the `GITHUB_CODEQL_ACTION_ANALYZE` in the codebase.
# Expect: Only occurrences of the new version `2.19.0`.
rg --type-not markdown --type-not txt $'GITHUB_CODEQL_ACTION_ANALYZE'

# Test 3: Search for the action usage in workflow files.
# Expect: The action version should be either `2.19.0` or `v2.19.0`.
rg --type yaml $'github/codeql-action/analyze@v?\d+\.\d+\.\d+'
Length of output: 183

Script:
#!/bin/bash
# Description: Verify the file content and search for CodeQL action references

# Test 1: Check the content of the versions file
echo "Content of versions/actions/GITHUB_CODEQL_ACTION_ANALYZE:"
cat versions/actions/GITHUB_CODEQL_ACTION_ANALYZE

# Test 2: Search for any CodeQL action references
echo -e "\nCodeQL action references:"
rg --type-not markdown --type-not txt "codeql-action" -i

# Test 3: List workflow files
echo -e "\nWorkflow files:"
fd -e yml -e yaml . .github/workflows

# Test 4: Display content of workflow files
echo -e "\nContent of workflow files:"
fd -e yml -e yaml . .github/workflows -x cat {}
Length of output: 217174

Script:
#!/bin/bash
# Description: Search for CodeQL action usage in workflow files

echo "Searching for CodeQL action usage in workflow files:"
grep -r "uses: github/codeql-action" .github/workflows

echo -e "\nChecking for specific CodeQL action version references:"
grep -r "uses: github/codeql-action@v" .github/workflows
Length of output: 611
versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD (1)

1-1: Version update looks good!

The version increment from 2.18.4 to 2.19.0 aligns with the PR objective of refactoring for the upcoming release v1.7.14.

versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF (1)

1-1: Verify compatibility and functionality of the updated version.

Updating to the latest version is a good practice to get the latest features, bug fixes, and security updates. The change from version 2.18.4 to 2.19.0 appears to be a minor version increment, which is unlikely to introduce breaking changes.

However, it's important to verify that the new version 2.19.0 is compatible with the project's dependencies and works as expected in the project's context.

versions/actions/GITHUB_ISSUE_METRICS (1)

1-1: LGTM!

The version number has been updated from 3.9.0 to 3.11.0, which is a common practice when releasing new versions of software. Without additional context or information about the specific changes, the version number update itself does not introduce any issues.

versions/K3S_VERSION (1)

1-1: Approve the K3S version update.

The update to K3S version v1.31.1-k3s1 aligns with the PR objectives and follows the expected version format.

Please ensure that:

The new version is compatible with other components and dependencies in the project.

Thorough testing is performed to validate the functionality and identify any potential regressions introduced by the version update.

.github/ISSUE_TEMPLATE/security_issue_report.md (1)

22-24: LGTM!

The version updates for Docker, Kubernetes, and Helm are consistent with the PR objective of updating dependencies. The changes are localized to the template and do not introduce any apparent issues.

.github/ISSUE_TEMPLATE/bug_report.md (1)

28-30: LGTM!

The version updates for Docker, Kubernetes, and Helm in the bug report template are consistent with the PR objectives. These changes ensure that the latest versions are referenced when reporting bugs, which helps in reproducing and resolving issues effectively.

k8s/external/minio/mb-job.yaml (1)

26-26: Optimize image pulling, but ensure cached images are current.

Changing imagePullPolicy to IfNotPresent can reduce unnecessary image pulls and improve startup time by using cached images.

However, please verify if there are processes in place to ensure cached images remain up-to-date with the latest version from the registry. Using outdated images may introduce bugs or vulnerabilities.

example/manifest/scylla/job.yaml (1)

26-26: Approve with caution: Ensure the image tag is immutable.

Changing imagePullPolicy to IfNotPresent can speed up pod startup times by avoiding unnecessary image pulls when the image already exists on the node.

However, be cautious if the cassandra:latest tag is not immutable. If the image is updated with the same tag, nodes may end up running different versions, leading to inconsistencies.

Consider using a specific immutable tag instead of latest to ensure consistent versions across all pods.

k8s/external/minio/deployment.yaml (1)

36-36: Optimize image pulling, but ensure version consistency.

Changing imagePullPolicy from Always to IfNotPresent can speed up pod start times by avoiding unnecessary image pulls when the required image already exists on the node.

However, please verify if there's a mechanism in place to ensure the required MinIO image version is present on the nodes. If the image is updated, IfNotPresent won't automatically pull the new version, potentially leading to version inconsistencies across the cluster.

Consider adding a comment to document the reasoning behind this change and any associated version management processes.

k8s/tools/cli/loadtest/job.yaml (1)

36-36: Approve the change to imagePullPolicy.

The change from Always to IfNotPresent can improve pod startup times and reduce network traffic, especially in environments where the image doesn't change frequently.

However, it's important to ensure that the image tag is updated whenever a new version of the image is pushed to the registry. Otherwise, outdated versions of the image may be used.

k8s/tools/cli/loadtest/cronjob.yaml (1)

40-40: The imagePullPolicy change is approved, but consider the trade-offs.

Changing imagePullPolicy from Always to IfNotPresent can help reduce unnecessary image pulls and improve resource usage, which is particularly beneficial for a CronJob that runs on a schedule.

However, please ensure this aligns with your image update strategy. If the vald-loadtest image is updated frequently, and it's crucial to always run the latest version, you might want to stick with Always.

dockers/agent/core/agent/Dockerfile (1)

43-43: LGTM!

The reordering of the CARGO_HOME environment variable declaration above the RUSTUP_HOME declaration is a minor change that does not affect the functionality of the Dockerfile. The value assigned to CARGO_HOME remains consistent with the previous configuration.

k8s/operator/helm/operator.yaml (1)

46-46: LGTM!

The change from Always to IfNotPresent for the imagePullPolicy is a good optimization. It avoids unnecessary image pulls when the image is already present on the node, reducing pod startup time and network bandwidth usage.

Just ensure that the image tag is properly managed to guarantee the desired version is used, as IfNotPresent will use the locally cached image if available, even if a newer version exists in the registry.

k8s/tools/benchmark/operator/deployment.yaml (1)

46-46: LGTM! Ensure the image tag is updated when a new version is released.

The change from Always to IfNotPresent aligns with the PR objective of optimizing image pulling behavior. This can speed up deployments and reduce network usage.

However, please ensure that the image tag (v1.7.13) is updated whenever a new version of the vald-benchmark-operator image is released. Otherwise, an outdated version may continue to be used.

internal/net/grpc/pool/pool_bench_test.go (2)

132-132: LGTM!

The change from grpc.DialContext to grpc.NewClient for establishing the gRPC connection is correct and aligns with the gRPC documentation.

189-189: LGTM!

Similar to the Benchmark_StaticDial function, the change from grpc.DialContext to grpc.NewClient for establishing the gRPC connection is correct and aligns with the gRPC documentation.

internal/observability/exporter/otlp/otlp.go (1)

34-34: LGTM!

The update to the newer version of the OpenTelemetry semantic conventions package looks good. It ensures compatibility with the latest standardized attributes and resource conventions.

Please make sure to review the changelog or release notes of the go.opentelemetry.io/otel/semconv package for version v1.26.0 to understand any changes, additions, or deprecations compared to the previous version v1.24.0.
internal/observability/trace/status.go (1)
24-24: Verify compatibility of the OpenTelemetry semantic conventions library version update.

The import statement for the OpenTelemetry semantic conventions package has been updated to version v1.26.0. This upgrade can provide access to new features, improvements, or bug fixes relevant to observability attributes.

Please ensure that the version update is compatible with the rest of the codebase and doesn't introduce any breaking changes. You can run the following script to search for potential incompatibilities:

Verification successful

The OpenTelemetry semantic conventions library version update appears to be compatible with the current usage.

After a thorough inspection of the codebase, the update from v1.24.0 to v1.26.0 of the OpenTelemetry semantic conventions library seems to be compatible with the current usage. The primary uses of the semconv package are for gRPC status codes and schema URL, which are standard OpenTelemetry concepts unlikely to have breaking changes in a minor version update.

Key observations:

Both files using the package (internal/observability/trace/status.go and internal/observability/exporter/otlp/otlp.go) have been updated to use v1.26.0 consistently.

The usage patterns focus on well-established OpenTelemetry concepts.

No deprecated or removed functions/constants are being used in the updated code.

While this analysis suggests compatibility, it's always a good practice to run the project's test suite to ensure there are no unforeseen issues.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Search for potential incompatibilities with the OpenTelemetry semantic conventions library version update.

# Test: Search for usage of the OpenTelemetry semantic conventions package. 
# Expect: Review the usage to ensure compatibility with the updated version.
rg --type go -A 5 $'semconv "go.opentelemetry.io/otel/semconv'
Length of output: 823

Script:
#!/bin/bash
# Description: Inspect usage of the OpenTelemetry semantic conventions package in the identified files and search for specific semconv usage in the codebase.

# Test 1: Examine the content of internal/observability/trace/status.go
echo "Content of internal/observability/trace/status.go:"
cat internal/observability/trace/status.go

# Test 2: Examine the content of internal/observability/exporter/otlp/otlp.go
echo -e "\n\nContent of internal/observability/exporter/otlp/otlp.go:"
cat internal/observability/exporter/otlp/otlp.go

# Test 3: Search for usage of semconv constants or functions in the entire codebase
echo -e "\n\nUsage of semconv constants or functions:"
rg --type go 'semconv\.' -A 3 -B 1
Length of output: 16759
k8s/index/job/save/cronjob.yaml (3)

56-56: LGTM!

Setting imagePullPolicy to IfNotPresent for the wait-for-agent initContainer is a good optimization. It will reduce unnecessary image pulls when the image already exists on the node.

68-68: LGTM!

Setting imagePullPolicy to IfNotPresent for the wait-for-discoverer initContainer is a good optimization. It will reduce unnecessary image pulls when the image already exists on the node.

81-81: LGTM!

Changing imagePullPolicy from Always to IfNotPresent for the vald-index-save container is a good optimization. It will reduce unnecessary image pulls when restarting the container if the image is already available on the node.

k8s/index/job/creation/cronjob.yaml (2)

68-68: Skipped comment as this is a duplicate of the previous imagePullPolicy comment.

81-81: Carefully consider if IfNotPresent is appropriate for the main job container.

The imagePullPolicy for the main vald-index-creation container has been changed from Always to IfNotPresent. This means:

The latest image won't be pulled if a local version already exists, even if updates are available in the registry.

The job may run using an outdated vdaas/vald-index-creation:nightly image.

Given this is the main job container, running an outdated version could lead to unexpected behavior or missing important bug fixes. The :nightly tag also implies frequent updates.

Please reevaluate if the benefits of reduced image pulling outweigh the risks of running outdated code for this critical container. Consider keeping Always if you want to ensure the latest nightly version is consistently used.

k8s/index/job/correction/cronjob.yaml (3)

56-56: LGTM!

The addition of the imagePullPolicy: IfNotPresent for the wait-for-agent init container looks good. This will help optimize image pulling by avoiding unnecessary pulls when the image already exists on the node.

68-68: Looks good!

Adding the imagePullPolicy: IfNotPresent for the wait-for-discoverer init container is a positive change. It aligns with the image pull optimization strategy applied to the other init container.

81-81: Approve the optimization.

Changing the imagePullPolicy from Always to IfNotPresent for the vald-index-correction container is a good optimization. It will reduce unnecessary image pulls when the image already exists on the node, improving efficiency.

This change aligns well with the pull policy updates made to the init containers.
k8s/index/operator/deployment.yaml (2)
49-49: LGTM!

Updating the checksum/configmap annotation is a good practice when the ConfigMap content changes. It helps in detecting and propagating the changes.

77-77: Verify that the required image version is pre-pulled on the nodes.

Changing imagePullPolicy to IfNotPresent can optimize pod startup time by avoiding unnecessary image pulls. However, please ensure that the required image version is always pre-pulled on the nodes. If the image gets updated in the registry with the same tag, the pods will continue using the old version until it's manually deleted from the nodes.

You can use the following script to verify that the required image version exists on the nodes:

If the above script returns an error, please pre-pull the image on the nodes using:

Repeat for all nodes in the cluster.

Verification successful

The imagePullPolicy: IfNotPresent change is appropriate and consistent with other components.

The change from Always to IfNotPresent for the vald-index-operator's imagePullPolicy is consistent with the configuration of other components in the Vald project. This change can indeed optimize pod startup time by avoiding unnecessary image pulls when the required version is already present on the node.

While the Helm chart documentation still lists the default imagePullPolicy as "Always" for most components, the specific configuration in the deployment file takes precedence. This allows for fine-tuned control over the pull policy for individual components.

To maintain consistency and clarity, consider updating the Helm chart documentation (charts/vald/README.md) to reflect this change if it's intended to be the new default behavior for the vald-index-operator.

Remember to ensure that your CI/CD pipeline or deployment process properly manages image updates to avoid using outdated images when changes are pushed to the registry with the same tag.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash

# Get the list of nodes
nodes=$(kubectl get nodes -o jsonpath='{.items[*].metadata.name}')

# Iterate over each node
for node in $nodes; do
  echo "Checking node: $node"
  
  # Check if the image exists on the node
  image_exists=$(ssh $node "crictl images | grep vdaas/vald-index-operator | grep nightly")
  
  if [[ -n $image_exists ]]; then
    echo "Image exists on the node"
  else
    echo "Image does not exist on the node"
    exit 1
  fi
done
Length of output: 144

Script:
kubectl drain <node-name> --ignore-daemonsets
docker pull vdaas/vald-index-operator:nightly
kubectl uncordon <node-name>
Length of output: 394

Script:
#!/bin/bash

# Search for references to the vald-index-operator image
echo "Searching for vald-index-operator image references:"
rg "vald-index-operator" -n

echo "\nSearching for image pull policy configurations:"
rg "imagePullPolicy" -n

echo "\nSearching for any documentation related to image versioning or pull policy:"
rg -i "image (versioning|pull policy)" -n --type md
Length of output: 115543
k8s/discoverer/deployment.yaml (1)

80-80: Approve the imagePullPolicy change with a reminder.

Changing imagePullPolicy from Always to IfNotPresent can improve pod start times by avoiding unnecessary image pulls. This is a good optimization, especially if the image version does not change frequently.

However, please ensure that the correct image version is always used when the pod starts. If an old or incorrect version is cached locally, it will be used instead of pulling the desired version. Consider the trade-offs and monitor the deployments to verify the expected behavior.

docs/user-guides/filtering-configuration.md (1)

156-156: LGTM!

The change from grpc.DialContext to grpc.NewClient for establishing the gRPC connection is valid and consistent with the gRPC documentation. The arguments passed to grpc.NewClient match those previously used with grpc.DialContext, ensuring a consistent connection configuration.

Tools

Markdownlint

156-156: Column: 1
Hard tabs

(MD010, no-hard-tabs)

tests/e2e/performance/max_vector_dim_test.go (1)

Line range hint 128-156: LGTM!

The changes in this code segment are appropriate and align with the gRPC documentation:

Replacing grpc.DialContext with grpc.NewClient is the recommended way to establish a gRPC client connection.

Moving the ctx variable declaration improves code readability without affecting the logic.

The error handling remains consistent, ensuring proper error propagation.

k8s/gateway/gateway/mirror/deployment.yaml (2)

58-58: Approve with caution when updating the image version.

Setting imagePullPolicy to IfNotPresent can speed up pod startup by avoiding unnecessary image pulls if the image already exists on the node.

However, be cautious when updating the image version, as IfNotPresent will not automatically pull the new version. You'll need to ensure the new image gets pulled manually to the nodes or change the policy to Always temporarily for deployment.

89-89: Approve but ensure image updates are handled carefully.

Changing imagePullPolicy from Always to IfNotPresent for the main vald-mirror-gateway container can reduce startup time and registry load, which is beneficial.

However, with IfNotPresent, there's a risk of running an outdated version when the image is updated, since it won't automatically pull the new version.

When deploying an image update to production, consider temporarily changing the policy back to Always, or manually ensuring the new image version is pulled to all nodes. This will prevent version inconsistencies across the deployment.

tests/e2e/operation/operation.go (4)

153-153: LGTM!

The updated call to getAgentClient without passing the ctx argument is consistent with the changes made to the method signature.

166-166: LGTM!

The updated call to getAgentClient without passing the ctx argument is consistent with the changes made to the method signature.

177-177: LGTM!

The updated call to getClient without passing the ctx argument is consistent with the changes made to the method signature.

185-185: LGTM!

The updated method signatures of getGRPCConn, getClient, and getAgentClient to remove the ctx parameter are consistent with the changes mentioned in the summary. The implementations have been updated accordingly to not pass the ctx argument when calling getGRPCConn.

Also applies to: 199-199, 208-208

k8s/manager/index/deployment.yaml (2)

62-62: Approve with caution when updating the image version.

The imagePullPolicy change to IfNotPresent can speed up pod startup by avoiding unnecessary image pulls if the image is already cached on the node.

However, be cautious when updating the busybox image version in the future. With IfNotPresent, Kubernetes won't automatically pull the new version unless the pod is recreated. Make sure to manually delete pods to force pulling the updated image when necessary.

96-96: Approve, but make sure to recreate the deployment when updating the image.

The imagePullPolicy change to IfNotPresent can speed up pod startup by avoiding unnecessary image pulls if the vald-manager-index image is already cached on the node.

However, since this is the main application container, it's critical to ensure it's running the expected version. When updating the vald-manager-index image version, make sure to recreate the deployment (e.g., by updating the pod template spec) to force pulling the new image. Otherwise, the old version will continue running.

k8s/gateway/gateway/lb/deployment.yaml (3)

61-61: LGTM!

Setting imagePullPolicy to IfNotPresent for the wait-for-discoverer init container is a good optimization. It will avoid unnecessary image pulls when the image already exists on the node, improving deployment speed.

73-73: Looks good!

Adding imagePullPolicy: IfNotPresent to the wait-for-agent init container is consistent with the previous change and will help optimize image pulling behavior.

104-104: Verify the impact of changing imagePullPolicy to IfNotPresent for the main application container.

While setting imagePullPolicy to IfNotPresent can help optimize image pulling, it's important to consider the implications for the vald-lb-gateway container:

If the vald-lb-gateway image is updated frequently, using IfNotPresent could result in running an outdated version of the image.

This might lead to inconsistencies or compatibility issues if the running image version doesn't match the expected version.

Please ensure that this change aligns with your desired deployment behavior and won't introduce any unintended consequences.
tests/e2e/operation/multi.go (1)
27-27: Verify the impact of removing the context parameter.

The context parameter has been removed from the getClient method call and the function signature. Please ensure that this change does not adversely affect the client behavior, especially regarding cancellation and deadline handling.

Verification successful

Removal of context parameter from getClient() verified as consistent and safe.

After thorough analysis, the removal of the context parameter from the getClient() method appears to be a consistent change across the codebase. The context is still properly utilized in subsequent operations after obtaining the client, preserving critical context-dependent functionalities. This change doesn't seem to adversely affect client behavior, cancellation, or deadline handling.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify if the `getClient` method and its usages handle cancellation and deadlines correctly after removing the context parameter.

# Test: Search for the `getClient` method declaration and its usages. Expect: No usage of context for cancellation or deadline handling.
rg --type go -A 5 $'func $c \*client$ getClient\('
rg --type go -A 5 $'c\.getClient\('
Length of output: 6672
example/client/main.go (1)

69-69: Verify compatibility and behavior of the new gRPC connection establishment method.

The change from grpc.DialContext to grpc.NewClient aligns with the updates in the gRPC library and API usage. However, please ensure that this new method is compatible with the rest of the codebase and doesn't introduce any unintended side effects.

Also, consider the implications of removing the ctx parameter, as it may impact the ability to cancel or timeout the connection establishment process. Ensure that this aligns with your desired connection management strategy.

example/client/mirror/main.go (1)

71-71: LGTM! The change from grpc.DialContext to grpc.NewClient aligns with gRPC best practices.

This update modernizes the gRPC client connection establishment to match the current recommendations in the gRPC documentation. It should not introduce any functional changes, as the connection parameters and error handling remain consistent.

Using grpc.NewClient is the preferred way to create a gRPC client connection in newer versions of the library.

example/client/agent/main.go (1)

70-70: LGTM! The gRPC connection method has been updated to align with the documentation and best practices.

The change from grpc.DialContext to grpc.NewClient is in line with the gRPC documentation, as grpc.DialContext has been deprecated. This update ensures compatibility with the latest gRPC version and avoids using deprecated methods.

The provided learning confirms this change:

In grpc-go, grpc.DialContext has been deprecated and grpc.NewClient should be used instead.

internal/net/grpc/errdetails/errdetails.go (2)

61-74: LGTM!

The changes to export the constants by updating their names to uppercase are appropriate and enhance their visibility outside the package.

237-316: Verify test coverage for the AnyToErrorDetail function.

The changes to the switch statement to reference the newly exported constants are correct.

However, the static analysis hints indicate that several lines in this function are not covered by tests, particularly the error handling paths for various cases.

To ensure the robustness of the error handling logic and prevent potential bugs or regressions, please consider adding more test cases to improve the coverage of the AnyToErrorDetail function. Focus on exercising the different error handling paths for each case in the switch statement.

If you need assistance in writing the additional test cases, I'd be happy to help. Let me know if you'd like me to provide some example test cases or open a GitHub issue to track this task.

Tools

GitHub Check: codecov/patch

[warning] 237-238: internal/net/grpc/errdetails/errdetails.go#L237-L238
Added lines #L237 - L238 were not covered by tests

[warning] 244-244: internal/net/grpc/errdetails/errdetails.go#L244
Added line #L244 was not covered by tests

[warning] 250-250: internal/net/grpc/errdetails/errdetails.go#L250
Added line #L250 was not covered by tests

[warning] 256-256: internal/net/grpc/errdetails/errdetails.go#L256
Added line #L256 was not covered by tests

[warning] 262-262: internal/net/grpc/errdetails/errdetails.go#L262
Added line #L262 was not covered by tests

[warning] 268-268: internal/net/grpc/errdetails/errdetails.go#L268
Added line #L268 was not covered by tests

[warning] 274-274: internal/net/grpc/errdetails/errdetails.go#L274
Added line #L274 was not covered by tests

[warning] 280-280: internal/net/grpc/errdetails/errdetails.go#L280
Added line #L280 was not covered by tests

[warning] 286-286: internal/net/grpc/errdetails/errdetails.go#L286
Added line #L286 was not covered by tests

[warning] 292-292: internal/net/grpc/errdetails/errdetails.go#L292
Added line #L292 was not covered by tests

[warning] 298-298: internal/net/grpc/errdetails/errdetails.go#L298
Added line #L298 was not covered by tests

[warning] 304-304: internal/net/grpc/errdetails/errdetails.go#L304
Added line #L304 was not covered by tests

[warning] 310-310: internal/net/grpc/errdetails/errdetails.go#L310
Added line #L310 was not covered by tests

[warning] 316-316: internal/net/grpc/errdetails/errdetails.go#L316
Added line #L316 was not covered by tests

docs/tutorial/get-started-with-faiss-agent.md (1)

281-281: LGTM!

The change from grpc.DialContext to grpc.NewClient looks good. Please ensure that all other occurrences of grpc.DialContext in the codebase are updated to maintain consistency.

The past comment about the sentence case is no longer applicable as the code has changed.

docs/tutorial/vald-agent-standalone-on-k8s.md (1)

239-239: LGTM!

The code change correctly replaces the deprecated grpc.DialContext with grpc.NewClient for establishing the gRPC connection, as per the gRPC documentation.
docs/tutorial/get-started.md (1)
336-336: Verify the impact of updating the gRPC connection method.

The change from grpc.DialContext to grpc.NewClient for establishing the gRPC connection is approved. However, please ensure that this change is consistently applied across the codebase and doesn't introduce any compatibility issues or breaking changes.

Run the following script to verify the usage of grpc.NewClient across the codebase:

Verification successful

Verification successful: grpc.NewClient usage is consistent across the codebase

The change from grpc.DialContext to grpc.NewClient has been successfully verified. The new method is consistently used throughout the codebase, including in tests, internal packages, and example code. No instances of the old grpc.DialContext method were found, indicating a thorough update. The usage and parameters of grpc.NewClient are consistent with the example provided in the tutorial.

This change appears to have been implemented correctly and comprehensively, with no apparent compatibility issues or inconsistencies.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of grpc.NewClient across the codebase.

# Test: Search for the function usage. Expect: Only occurrences of grpc.NewClient.
rg --type go -A 5 $'grpc\.NewClient'
Length of output: 4413
docs/user-guides/client-api-config.md (5)
473-473: Verify the impact of removing the context parameter.

The method signature change from grpc.DialContext(ctx, target) to grpc.NewClient(target) looks good. However, please ensure that removing the context parameter does not affect the ability to cancel or timeout the connection attempt.

To verify the impact, search for usages of the conn variable returned by grpc.NewClient(target) and check if any of them rely on the context for cancellation or timeout.
#!/bin/bash
# Description: Verify if the `conn` variable is used with context for cancellation or timeout.

# Test: Search for usages of the `conn` variable. Expect: No usage with context for cancellation or timeout.
rg --type markdown $'conn, err := grpc.NewClient$target$' -A 10 
Tools

Markdownlint

473-473: Column: 1
Hard tabs

(MD010, no-hard-tabs)

165-165: Verify the impact of removing the context parameter.

The method signature change from grpc.DialContext(ctx, target) to grpc.NewClient(target) looks good. However, please ensure that removing the context parameter does not affect the ability to cancel or timeout the connection attempt.

To verify the impact, search for usages of the conn variable returned by grpc.NewClient(target) and check if any of them rely on the context for cancellation or timeout.
#!/bin/bash
# Description: Verify if the `conn` variable is used with context for cancellation or timeout.

# Test: Search for usages of the `conn` variable. Expect: No usage with context for cancellation or timeout.
rg --type markdown $'conn, err := grpc.NewClient$target$' -A 10
Tools

Markdownlint

165-165: Column: 1
Hard tabs

(MD010, no-hard-tabs)

289-289: Verify the impact of removing the context parameter.

The method signature change from grpc.DialContext(ctx, target) to grpc.NewClient(target) looks good. However, please ensure that removing the context parameter does not affect the ability to cancel or timeout the connection attempt.

To verify the impact, search for usages of the conn variable returned by grpc.NewClient(target) and check if any of them rely on the context for cancellation or timeout.
#!/bin/bash  
# Description: Verify if the `conn` variable is used with context for cancellation or timeout.

# Test: Search for usages of the `conn` variable. Expect: No usage with context for cancellation or timeout.
rg --type markdown $'conn, err := grpc.NewClient$target$' -A 10
Tools

Markdownlint

289-289: Column: 1
Hard tabs

(MD010, no-hard-tabs)

656-656: Verify the impact of removing the context parameter.

The method signature change from grpc.DialContext(ctx, target) to grpc.NewClient(target) looks good. However, please ensure that removing the context parameter does not affect the ability to cancel or timeout the connection attempt.

To verify the impact, search for usages of the conn variable returned by grpc.NewClient(target) and check if any of them rely on the context for cancellation or timeout.

Verification successful

Removal of context parameter from grpc.NewClient(target) is appropriate

After analyzing the usage of the conn variable, I can confirm that removing the context parameter from the connection establishment does not negatively impact the functionality. The context is still being used in individual client operations (Insert, Update, Upsert, Search, Remove), allowing for more granular control over timeouts and cancellations per operation. This change appears to be an improvement in the API design.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify if the `conn` variable is used with context for cancellation or timeout.

# Test: Search for usages of the `conn` variable. Expect: No usage with context for cancellation or timeout.
rg --type markdown $'conn, err := grpc.NewClient$target$' -A 10
Length of output: 3539
Tools

Markdownlint

656-656: Column: 1
Hard tabs

(MD010, no-hard-tabs)

49-49: Verify the impact of removing the context parameter.

The method signature change from grpc.DialContext(ctx, target) to grpc.NewClient(target) looks good. However, please ensure that removing the context parameter does not affect the ability to cancel or timeout the connection attempt.

To verify the impact, search for usages of the conn variable returned by grpc.NewClient(target) and check if any of them rely on the context for cancellation or timeout.

Verification successful

Removal of context parameter appears safe, but verify grpc.NewClient implementation

The removal of the context parameter from grpc.NewClient(target) doesn't seem to impact the ability to cancel or timeout operations. The context is still being used in subsequent Vald client method calls (Insert, Update, Upsert, Search, Remove), which suggests that cancellation and timeout functionality is maintained at the operation level.

However, to ensure complete safety:

Verify the implementation of grpc.NewClient to confirm it doesn't introduce any limitations compared to the previous grpc.DialContext.

Check if there are any connection-level timeout or cancellation requirements that might be affected by this change.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify if the `conn` variable is used with context for cancellation or timeout.

# Test: Search for usages of the `conn` variable. Expect: No usage with context for cancellation or timeout.
rg --type markdown $'conn, err := grpc.NewClient$target$' -A 10
Length of output: 3539
Tools

Markdownlint

49-49: Column: 1
Hard tabs

(MD010, no-hard-tabs)
cmd/index/operator/sample.yaml (4)

202-202: Verify the intended image pull behavior for the vald-readreplica-rotate container.

Changing imagePullPolicy from Always to IfNotPresent can speed up pod startup times by avoiding unnecessary image pulls. However, it also means that pods may start using an outdated version of the image if the image tag has been updated in the registry but not pulled locally.

Please ensure this aligns with the intended behavior. If you always want to use the latest image, consider using a unique tag for each image version instead of relying on the Always pull policy.

338-338: LGTM!

The same analysis and recommendation from the previous imagePullPolicy change apply here as well.

453-453: LGTM!

The same analysis and recommendation from the previous imagePullPolicy changes apply here as well.

568-568: LGTM!

The same analysis and recommendation from the previous imagePullPolicy changes apply here as well.

tests/e2e/operation/stream.go (13)

96-96: Verify if removing ctx from getClient is intentional.

The ctx parameter has been removed from the getClient method call. Please ensure that this change doesn't affect the ability to propagate cancellation signals or deadlines to the underlying gRPC operations.

296-296: Verify if removing ctx from getClient is intentional.

The ctx parameter has been removed from the getClient method call. Please ensure that this change doesn't affect the ability to propagate cancellation signals or deadlines to the underlying gRPC operations.

416-416: Verify if removing ctx from getClient is intentional.

The ctx parameter has been removed from the getClient method call. Please ensure that this change doesn't affect the ability to propagate cancellation signals or deadlines to the underlying gRPC operations.

534-534: Verify if removing ctx from getClient is intentional.

The ctx parameter has been removed from the getClient method call. Please ensure that this change doesn't affect the ability to propagate cancellation signals or deadlines to the underlying gRPC operations.

640-640: Verify if removing ctx from getClient is intentional.

The ctx parameter has been removed from the getClient method call. Please ensure that this change doesn't affect the ability to propagate cancellation signals or deadlines to the underlying gRPC operations.

743-743: Verify if removing ctx from getClient is intentional.

The ctx parameter has been removed from the getClient method call. Please ensure that this change doesn't affect the ability to propagate cancellation signals or deadlines to the underlying gRPC operations.

847-847: Verify if removing ctx from getClient is intentional.

The ctx parameter has been removed from the getClient method call. Please ensure that this change doesn't affect the ability to propagate cancellation signals or deadlines to the underlying gRPC operations.

949-949: Verify if removing ctx from getClient is intentional.

The ctx parameter has been removed from the getClient method call. Please ensure that this change doesn't affect the ability to propagate cancellation signals or deadlines to the underlying gRPC operations.

1032-1032: Verify if removing ctx from getClient is intentional.

The ctx parameter has been removed from the getClient method call. Please ensure that this change doesn't affect the ability to propagate cancellation signals or deadlines to the underlying gRPC operations.

1050-1050: Verify if removing ctx from getClient is intentional.

The ctx parameter has been removed from the getClient method call. Please ensure that this change doesn't affect the ability to propagate cancellation signals or deadlines to the underlying gRPC operations.

1077-1077: Verify if removing ctx from getClient is intentional.

The ctx parameter has been removed from the getClient method call. Please ensure that this change doesn't affect the ability to propagate cancellation signals or deadlines to the underlying gRPC operations.

1099-1099: Verify if removing ctx from getClient is intentional.

The ctx parameter has been removed from the getClient method call. Please ensure that this change doesn't affect the ability to propagate cancellation signals or deadlines to the underlying gRPC operations.

1189-1189: Verify if removing ctx from getClient is intentional.

The ctx parameter has been removed from the getClient method call. Please ensure that this change doesn't affect the ability to propagate cancellation signals or deadlines to the underlying gRPC operations.

k8s/index/operator/configmap.yaml (5)

28-28: LGTM!

The YAML configuration for the index operator looks good.

Line range hint 278-278: Verify image version on nodes.

The change to imagePullPolicy: IfNotPresent looks good as it can help optimize resource usage. However, please ensure that the desired vald-readreplica-rotate image version is already present on the nodes to avoid any unexpected behavior.

Line range hint 384-384: Verify image version on nodes.

The change to imagePullPolicy: IfNotPresent looks good as it can help optimize resource usage. However, please ensure that the desired vald-index-creation image version is already present on the nodes to avoid any unexpected behavior.

Line range hint 402-402: LGTM!

The changes to imagePullPolicy: IfNotPresent for the init containers look good. Since these containers use the busybox image, which is relatively stable, using IfNotPresent should help speed up pod startup times without causing issues.

Also applies to: 413-413

Line range hint 508-508: Verify image version on nodes.

The change to imagePullPolicy: IfNotPresent looks good as it can help optimize resource usage. However, please ensure that the desired vald-index-save image version is already present on the nodes to avoid any unexpected behavior.

Makefile (1)

682-703: LGTM!

The updated usearch/install function looks good:

It clones the usearch repository with the correct version and depth.

The build is configured with appropriate options like enabling specific features, setting compiler flags, and specifying the install prefix.

The project is built and installed correctly.

The resulting libraries are copied to the designated library path.

The temporary build directory is cleaned up.

Overall, the installation process follows best practices and looks correct.

internal/net/grpc/client.go (1)

168-168: Verify the impact of removing the grpc.WithBlock() option.

The removal of the grpc.WithBlock() option simplifies the connection initiation process by eliminating the blocking behavior. However, please ensure that this change aligns with the expected behavior and does not introduce any unintended consequences in the codebase.

Run the following script to verify the usage of g.Connect:

Tools

GitHub Check: codecov/patch

[warning] 168-168: internal/net/grpc/client.go#L168
Added line #L168 was not covered by tests

go.mod (2)

7-8: LGTM!

The version updates for various dependencies look good. The changes are mostly minor or patch releases, which should be safe and backward-compatible.

Also applies to: 11-13, 15-15, 17-17, 41-42, 48-55, 57-68, 85-85, 173-173, 195-195, 199-199, 240-240, 317-317, 322-322, 411-411

Line range hint 1-529: Summary Consistency

The code changes align with the high-level description provided in the AI-generated summary. The summary accurately captures the overall nature of the updates without going into the granular details of each version change.

charts/vald/templates/_helpers.tpl (1)

702-702: LGTM!

The addition of the imagePullPolicy field for init containers is a good practice. Using a variable for the value allows flexibility in customizing the policy.

charts/vald/values.yaml (4)

1078-1078: Verify if using IfNotPresent aligns with the desired image update strategy for the wait-for-discoverer initContainer.

Using imagePullPolicy: IfNotPresent can speed up pod startup time by skipping the image pull if it already exists locally. However, this risks running an outdated version if the image tag is updated in the registry but the node still has an old cached version.

Consider if this aligns with your strategy for updating the busybox image used in this initContainer. You may want to use Always instead if you expect the image to be updated regularly and want to ensure the latest version is always pulled.

1084-1084: Verify if using IfNotPresent aligns with the desired image update strategy for the wait-for-agent initContainer.

The same consideration as the previous comment applies here as well. Using IfNotPresent has benefits for startup time but risks running outdated images.

Please ensure the imagePullPolicy choice aligns with your intended update strategy for the busybox image across all wait-for type initContainers.

3140-3140: Ensure consistent imagePullPolicy for wait-for initContainers.

As mentioned in the previous comments, please verify that using IfNotPresent for the wait-for-agent initContainer aligns with your desired image update strategy.

The imagePullPolicy should be consistent across all the wait-for initContainers to ensure predictable behavior when deploying new versions of the busybox image.

3146-3146: Ensure consistent imagePullPolicy for wait-for initContainers.

This is a continuation of the previous comments regarding the imagePullPolicy for wait-for type initContainers.

Please make sure the IfNotPresent policy is intentionally chosen here for the wait-for-discoverer initContainer, considering the trade-offs discussed earlier. The policy should ideally be the same for all similar initContainers to maintain consistency.

internal/net/grpc/interceptor/server/logging/accesslog.go (1)

22-22: Imports and constants look good.

The added imports (fmt, json, status) and the new constant rpcFailedMessage are appropriate and necessary for the new functionality.

Also applies to: 26-26, 29-29, 38-38
internal/net/grpc/status/status.go (1)
21-24: Potential Compatibility Issue with Go 1.21 Dependencies

The imports of cmp and slices packages (lines 21-24) are part of the Go 1.21 standard library. Similarly, the strings package (line 33) may require a specific Go version for certain functions used.

Please ensure that the project's minimum Go version is set to 1.21 or later to avoid compatibility issues with environments using older Go versions.

Run the following script to verify the Go version specified in your project:

Also applies to: 33-33

Verification successful

Go Version Compatibility Verified

The project's go.mod file specifies Go version 1.23.1, which is higher than the required version 1.21 mentioned in the review comment. This ensures compatibility with the cmp, slices, and strings packages used in the code. No further action is needed regarding Go version compatibility.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check if 'go.mod' specifies Go version 1.21 or later.

# Expectation: The 'go.mod' file should specify 'go 1.21' or greater.
grep '^go ' go.mod
Length of output: 30
.gitfiles (4)
1957-1960: Ensure Rust meta Service Aligns With Project Conventions

The new Rust files for the meta service should adhere to existing architectural patterns and coding conventions used in the project for consistency.

170-171: Ensure Generated gRPC Code Matches Proto Definitions

Please verify that meta.pb.go and meta_vtproto.pb.go are generated from the latest meta.proto file to prevent any mismatches between the proto definitions and the generated code.

You can run the following command to regenerate the protobuf files and check for differences:

Verification successful

Generated gRPC Code Appears to Match Proto Definitions

Based on the available evidence, it appears that meta.pb.go and meta_vtproto.pb.go are likely generated from the latest meta.proto file:

All three files (meta.proto, meta.pb.go, and meta_vtproto.pb.go) have the same timestamp (Sep 21 06:15).

The meta.pb.go file contains the comment "Code generated by protoc-gen-go. DO NOT EDIT."

While we cannot perform a full content comparison due to sandbox limitations, these indicators strongly suggest that the generated code is up-to-date with the proto definitions.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Regenerate protobuf files and check for any changes
buf generate
git diff --exit-code apis/grpc/v1/meta/meta.pb.go apis/grpc/v1/meta/meta_vtproto.pb.go
Length of output: 184

Script:
#!/bin/bash
# Check existence and timestamps of proto and generated files
echo "Checking file existence and timestamps:"
ls -l apis/proto/v1/meta/meta.proto apis/grpc/v1/meta/meta.pb.go apis/grpc/v1/meta/meta_vtproto.pb.go 2>/dev/null

echo -e "\nChecking for generation info in meta.pb.go:"
head -n 20 apis/grpc/v1/meta/meta.pb.go 2>/dev/null | grep -i "generated"

echo -e "\nSearching for protobuf generation scripts:"
fd -e sh -e yaml -e yml -e json | xargs grep -i "protoc\|buf generate" 2>/dev/null
Length of output: 103542
1034-1034: Confirm Comprehensive Error Handling in usearch

Verify that all potential errors within the usearch algorithm are properly captured and returned using the defined error types in usearch.go.

You can search for error returns in the usearch code:

Verification successful

To further verify the comprehensive error handling in the usearch algorithm, we need to gather more information. Let's run some additional scripts to examine the public functions and error definitions.

To complete our verification of comprehensive error handling in the usearch algorithm, we need to gather more information about the content of usearch.go and the usage of UsearchError. Let's run additional scripts to examine these aspects.

Comprehensive Error Handling Confirmed in usearch

The verification process has confirmed that error handling in the usearch implementation is indeed comprehensive and well-implemented. Key observations include:

Consistent use of UsearchError throughout the codebase for various error scenarios.

Descriptive error messages providing context for each failure case.

Error handling present in all public methods of the Usearch interface.

Proper use of the custom errors package for creating and wrapping errors.

The implementation covers error cases for:

Initialization and configuration

Index operations (add, remove, search)

Resource management (reserve, close)

File operations (save, load)

This level of error handling should provide developers with clear information about potential issues and their causes, facilitating easier debugging and maintenance.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for error handling patterns in usearch code
rg --type go 'return.*errors\.' internal/core/algorithm/usearch/
Length of output: 2830

Script:
#!/bin/bash
# List all public functions in usearch.go
echo "Public functions in usearch.go:"
ast-grep --lang go --pattern 'func ($id:$_) $_($_) $_' internal/core/algorithm/usearch/usearch.go

# Show content of errors package related to usearch errors
echo -e "\nUsearch-related error definitions:"
rg --type go '(type|func).*Error' internal/errors/
Length of output: 7036

Script:
#!/bin/bash
# Display the content of usearch.go
echo "Content of usearch.go:"
cat internal/core/algorithm/usearch/usearch.go

# Search for usage of UsearchError in usearch implementation
echo -e "\nUsage of UsearchError:"
rg --type go 'UsearchError' internal/core/algorithm/usearch/
Length of output: 8593
226-226: Verify Consistency Between Swagger and Proto Definitions

Ensure that meta.swagger.json accurately reflects the definitions in meta.proto. Inconsistencies could lead to incorrect API documentation.

You can run the following command to regenerate the Swagger file and check for differences:

Verification successful

Consistency Verified: Swagger Accurately Reflects Proto Definitions

The meta.swagger.json file correctly represents the definitions in meta.proto. Key findings:

Both files define the same methods: Get, Set, and Delete.

HTTP methods and paths in Swagger align with gRPC definitions in proto.

Parameter structures are consistent (e.g., Set method using POST and body parameter).

No discrepancies were found between the two files.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Generate Swagger documentation and check for discrepancies
buf generate --path apis/proto/v1/meta/meta.proto
git diff --exit-code apis/swagger/v1/meta/meta.swagger.json
Length of output: 184

Script:
#!/bin/bash
# Check if meta.swagger.json exists and examine its content
echo "Checking meta.swagger.json:"
if [ -f apis/swagger/v1/meta/meta.swagger.json ]; then
    echo "File exists. Displaying first 20 lines:"
    head -n 20 apis/swagger/v1/meta/meta.swagger.json
else
    echo "File does not exist."
fi

echo -e "\nChecking meta.proto:"
if [ -f apis/proto/v1/meta/meta.proto ]; then
    echo "File exists. Displaying first 20 lines:"
    head -n 20 apis/proto/v1/meta/meta.proto
else
    echo "File does not exist."
fi

echo -e "\nChecking recent changes:"
git log -n 5 --pretty=format:"%h - %s" -- apis/swagger/v1/meta/meta.swagger.json apis/proto/v1/meta/meta.proto
Length of output: 2056

Script:
#!/bin/bash
# Examine more content of meta.proto and meta.swagger.json
echo "Examining meta.proto for service definitions:"
grep -n "service" apis/proto/v1/meta/meta.proto -A 10

echo -e "\nExamining meta.swagger.json for paths:"
grep -n '"paths"' apis/swagger/v1/meta/meta.swagger.json -A 20

echo -e "\nComparing method names:"
echo "Methods in proto:"
grep "rpc" apis/proto/v1/meta/meta.proto | awk '{print $2}'
echo "Methods in swagger:"
grep '"operationId"' apis/swagger/v1/meta/meta.swagger.json | awk -F'"' '{print $4}' | awk -F'_' '{print $2}'
Length of output: 1654

coderabbitai · 2024-09-21T06:41:44Z

example/client/go.mod

@@ -14,7 +14,7 @@ replace (
 	google.golang.org/genproto => google.golang.org/genproto v0.0.0-20240903143218-8af14fe29dc1
 	google.golang.org/genproto/googleapis/api => google.golang.org/genproto/googleapis/api v0.0.0-20240903143218-8af14fe29dc1
 	google.golang.org/genproto/googleapis/rpc => google.golang.org/genproto/googleapis/rpc v0.0.0-20240903143218-8af14fe29dc1
-	google.golang.org/grpc => google.golang.org/grpc v1.66.2
+	google.golang.org/grpc => google.golang.org/grpc v1.67.0


Update the required version of google.golang.org/grpc to match the replaced version.

There is an inconsistency between the replaced version (v1.67.0) and the required version (v1.66.0) of the google.golang.org/grpc dependency. To avoid potential issues, update the required version to match the replaced version.

Apply this diff to fix the inconsistency:

require ( github.com/kpango/fuid v0.0.0-20221203053508-503b5ad89aa1 github.com/kpango/glg v1.6.14 github.com/vdaas/vald-client-go v1.7.13 gonum.org/v1/hdf5 v0.0.0-20210714002203-8c5d23bc6946 - google.golang.org/grpc v1.66.0 + google.golang.org/grpc v1.67.0 )

Also applies to: 26-26

coderabbitai · 2024-09-21T06:41:44Z

internal/net/grpc/interceptor/server/logging/accesslog.go

@@ -88,9 +115,9 @@

 		if err != nil {
 			entity.Error = err
-			log.Warn(rpcCompletedMessage, entity)
+			log.Warn(rpcFailedMessage, entity.String())


Increase test coverage for new logging code paths.

The added logging statements in AccessLogInterceptor and AccessLogStreamInterceptor are not covered by tests, as indicated by static analysis tools. Ensuring these paths are tested will improve code reliability and prevent future regressions.

Consider adding unit tests that simulate both successful and failed RPC calls to verify that the logging behaves as expected.

Also applies to: 120-120, 166-166, 168-168

Tools

GitHub Check: codecov/patch

[warning] 118-118: internal/net/grpc/interceptor/server/logging/accesslog.go#L118
Added line #L118 was not covered by tests

internal/net/grpc/status/status.go

.gitfiles

coderabbitai · 2024-09-21T06:41:45Z

.gitfiles

+k8s/agent/clusterrole.yaml
+k8s/agent/clusterrolebinding.yaml
+k8s/agent/daemonset.yaml
+k8s/agent/deployment.yaml
+k8s/agent/faiss/configmap.yaml
+k8s/agent/hpa.yaml
+k8s/agent/networkpolicy.yaml
 k8s/agent/ngt/configmap.yaml
 k8s/agent/pdb.yaml
 k8s/agent/priorityclass.yaml
+k8s/agent/serviceaccount.yaml
+k8s/agent/sidecar/configmap.yaml
+k8s/agent/sidecar/svc.yaml


Review Kubernetes RBAC Configurations for Security

The added Kubernetes RBAC files grant permissions via ClusterRole and ClusterRoleBinding. Ensure that they follow the principle of least privilege by granting only necessary permissions.

Consider revising the RBAC definitions to limit permissions. For example:

apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: name: agent-role rules: - apiGroups: [""] - resources: ["pods", "services"] + resources: ["pods"] verbs: ["get", "list", "watch"]

Committable suggestion was skipped due to low confidence.

coderabbitai

Actionable comments posted: 6

Outside diff range and nitpick comments (7)

k8s/operator/helm/operator.yaml (1)

46-46: Approve with suggestion: Use an immutable image tag.

The change from Always to IfNotPresent for the imagePullPolicy is approved, as it can speed up pod startup times by avoiding unnecessary image pulls.

However, to prevent potential inconsistencies between the locally cached image and the version in the registry, consider using an immutable image tag (e.g., a specific semantic version or a commit SHA) instead of a mutable tag like latest or v1.7.13.

k8s/agent/statefulset.yaml (1)

79-79: The imagePullPolicy change is reasonable but warrants caution.

Changing imagePullPolicy to IfNotPresent can help reduce unnecessary pulls and startup time. However, be cautious with moving tags like nightly:

If the nightly tag is not reliably updated on each build, this could cause inconsistent versions across the cluster over time.

Nodes that are long-running may become increasingly stale.

Consider using explicit versioned image tags (e.g. 1.7.13) to ensure the desired version is pulled.

k8s/discoverer/deployment.yaml (1)

80-80: Approve with caution: Ensure unique image tags for each version.

Changing imagePullPolicy to IfNotPresent can reduce unnecessary image pulls and improve container startup efficiency when the image already exists on the node.

However, be cautious about relying on the image tag always pointing to the desired version. If the same tag is reused for different versions, it may lead to inconsistencies across nodes.

Consider using unique tags for each version of the image to ensure consistency and avoid potential issues from reusing tags.
k8s/gateway/gateway/mirror/deployment.yaml (1)
89-89: Approve, but use an immutable image tag and exercise caution when updating.

Changing imagePullPolicy from Always to IfNotPresent aligns with best practices and can reduce unnecessary load on the container registry. However, I recommend using an immutable image tag (e.g., a specific semantic version or commit SHA) instead of the nightly tag. This makes it explicit which code version each pod is running.

Also, be cautious when updating the image version. With imagePullPolicy: IfNotPresent, the old version will continue to be used until the pod is recreated. Consider rolling out updates to avoid inconsistencies.

To use an immutable image tag, update the image field:
- image: "vdaas/vald-mirror-gateway:nightly"
+ image: "vdaas/vald-mirror-gateway:v1.2.3"
Replace v1.2.3 with the specific version you want to deploy.
.gitfiles (3)

208-208: Add Comments to meta.proto for Clarity

Consider adding comments to all message fields and RPC methods in meta.proto to enhance readability and maintainability. Well-documented code helps other developers understand the purpose and usage of each component.

1034-1034: Maintain Consistency in Error Definitions for usearch

Check that usearch.go in internal/errors follows the project's conventions for error definitions. Consistent error handling across modules improves code maintainability and debugging efficiency.

1442-1449: Review gateway/filter Kubernetes Manifests

New Kubernetes manifests for the gateway/filter component have been added. Please ensure:

Security Policies: Network policies and pod security contexts are correctly configured to prevent unauthorized access.

Resource Allocation: Resource requests and limits are set to optimize performance without overconsumption.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between bcebbdc and 3bea900.

Files ignored due to path filters (6)

apis/grpc/v1/agent/sidecar/sidecar_vtproto.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
apis/grpc/v1/payload/payload.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
apis/grpc/v1/rpc/errdetails/error_details.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
example/client/go.sum is excluded by !**/*.sum
go.sum is excluded by !**/*.sum
rust/Cargo.lock is excluded by !**/*.lock

Files selected for processing (65)

.gitfiles (9 hunks)
.github/ISSUE_TEMPLATE/bug_report.md (1 hunks)
.github/ISSUE_TEMPLATE/security_issue_report.md (1 hunks)
.github/PULL_REQUEST_TEMPLATE.md (3 hunks)
Makefile (2 hunks)
charts/vald/templates/_helpers.tpl (1 hunks)
charts/vald/values.yaml (7 hunks)
cmd/index/operator/sample.yaml (4 hunks)
dockers/agent/core/agent/Dockerfile (1 hunks)
docs/tutorial/get-started-with-faiss-agent.md (1 hunks)
docs/tutorial/get-started.md (1 hunks)
docs/tutorial/vald-agent-standalone-on-k8s.md (1 hunks)
docs/user-guides/client-api-config.md (5 hunks)
docs/user-guides/filtering-configuration.md (1 hunks)
example/client/agent/main.go (1 hunks)
example/client/go.mod (2 hunks)
example/client/main.go (1 hunks)
example/client/mirror/main.go (1 hunks)
example/manifest/scylla/job.yaml (1 hunks)
go.mod (11 hunks)
hack/docker/gen/main.go (1 hunks)
internal/net/grpc/client.go (1 hunks)
internal/net/grpc/errdetails/errdetails.go (2 hunks)
internal/net/grpc/interceptor/server/logging/accesslog.go (5 hunks)
internal/net/grpc/pool/pool.go (4 hunks)
internal/net/grpc/pool/pool_bench_test.go (2 hunks)
internal/net/grpc/status/status.go (4 hunks)
internal/observability/exporter/otlp/otlp.go (1 hunks)
internal/observability/trace/status.go (1 hunks)
k8s/agent/statefulset.yaml (1 hunks)
k8s/discoverer/deployment.yaml (1 hunks)
k8s/external/minio/deployment.yaml (1 hunks)
k8s/external/minio/mb-job.yaml (1 hunks)
k8s/gateway/gateway/lb/deployment.yaml (3 hunks)
k8s/gateway/gateway/mirror/deployment.yaml (2 hunks)
k8s/index/job/correction/cronjob.yaml (3 hunks)
k8s/index/job/creation/cronjob.yaml (3 hunks)
k8s/index/job/save/cronjob.yaml (3 hunks)
k8s/index/operator/configmap.yaml (1 hunks)
k8s/index/operator/deployment.yaml (2 hunks)
k8s/manager/index/deployment.yaml (3 hunks)
k8s/operator/helm/operator.yaml (1 hunks)
k8s/tools/benchmark/operator/deployment.yaml (1 hunks)
k8s/tools/cli/loadtest/cronjob.yaml (1 hunks)
k8s/tools/cli/loadtest/job.yaml (1 hunks)
tests/e2e/operation/multi.go (8 hunks)
tests/e2e/operation/operation.go (4 hunks)
tests/e2e/operation/stream.go (13 hunks)
tests/e2e/performance/max_vector_dim_test.go (2 hunks)
versions/CHAOS_MESH_VERSION (1 hunks)
versions/DOCKER_VERSION (1 hunks)
versions/HELM_VERSION (1 hunks)
versions/K3S_VERSION (1 hunks)
versions/KUBECTL_VERSION (1 hunks)
versions/OPERATOR_SDK_VERSION (1 hunks)
versions/PROMETHEUS_STACK_VERSION (1 hunks)
versions/PROTOBUF_VERSION (1 hunks)
versions/REVIEWDOG_VERSION (1 hunks)
versions/actions/ACTIONS_SETUP_NODE (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_ANALYZE (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_INIT (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF (1 hunks)
versions/actions/GITHUB_ISSUE_METRICS (1 hunks)
versions/actions/PETER_EVANS_CREATE_PULL_REQUEST (1 hunks)

Files skipped from review due to trivial changes (9)

.github/PULL_REQUEST_TEMPLATE.md
versions/CHAOS_MESH_VERSION
versions/HELM_VERSION
versions/KUBECTL_VERSION
versions/PROTOBUF_VERSION
versions/REVIEWDOG_VERSION
versions/actions/ACTIONS_SETUP_NODE
versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF
versions/actions/PETER_EVANS_CREATE_PULL_REQUEST

Additional context used

Learnings (2)

docs/tutorial/vald-agent-standalone-on-k8s.md (1)

Learnt from: kpango
PR: vdaas/vald#2638
File: docs/tutorial/vald-agent-standalone-on-k8s.md:239-239
Timestamp: 2024-09-21T05:35:36.818Z
Learning: In grpc-go, `grpc.DialContext` has been deprecated and `grpc.NewClient` should be used instead.

example/client/agent/main.go (1)

Learnt from: kpango
PR: vdaas/vald#2638
File: docs/tutorial/vald-agent-standalone-on-k8s.md:239-239
Timestamp: 2024-09-21T05:35:36.818Z
Learning: In grpc-go, `grpc.DialContext` has been deprecated and `grpc.NewClient` should be used instead.

Additional comments not posted (129)

versions/OPERATOR_SDK_VERSION (1)

1-1: Provide more context about the version update.

Updating to the latest version of the Operator SDK is a good practice to incorporate bug fixes, security patches, and new features. However, it would be helpful to provide more information about the changes introduced in the interim releases (v1.34, v1.35, v1.36) and their impact on the project.

Please ensure that the version update is compatible with the project's dependencies and does not introduce any breaking changes.

versions/PROMETHEUS_STACK_VERSION (1)

1-1: Verify compatibility and impact of the Prometheus stack version update.

The version update from 62.6.0 to 62.7.0 looks good. However, please ensure that:

The updated version is compatible with the rest of the system.

There are no breaking changes introduced by the new version that could impact the functionality or control flow of the system.

versions/actions/GITHUB_CODEQL_ACTION_ANALYZE (1)

1-1: Verify compatibility and review changelog for the version update.

Updating the GITHUB_CODEQL_ACTION_ANALYZE version from 2.18.4 to 2.19.0 aligns with the PR objective of updating project dependencies.

However, it's important to review the changelog or release notes to:

Understand what new features, bug fixes, or improvements are included in this version.

Verify that the update does not introduce any breaking changes or compatibility issues.

Identify any relevant changes that may require additional code modifications or migration steps.

If no issues are found, then this version update looks good to merge.

versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD (1)

1-1: Verify compatibility and test thoroughly.

Updating to the latest version is a good practice to incorporate the latest features, performance improvements, and bug fixes.

However, please ensure that:

The new version 2.19.0 is compatible with the rest of the codebase and does not introduce any breaking changes.

Thorough testing is performed to validate that the new version works as expected in your environment.

versions/actions/GITHUB_CODEQL_ACTION_INIT (1)

1-1: LGTM!

The version update from 2.18.4 to 2.19.0 looks good. It aligns with the PR objective of updating project dependencies to their latest versions.

versions/actions/GITHUB_ISSUE_METRICS (1)

1-1: Version number update looks good!

The version number has been incremented from 3.9.0 to 3.11.0, which suggests that the codebase has undergone modifications or improvements that warrant a new version release. This change is important for tracking the software's evolution and indicating the introduction of enhancements, bug fixes, or new features.

versions/DOCKER_VERSION (1)

1-1: Provide more context for the Docker version update.

The Docker version has been updated from v27.2.1 to v27.3.1. While updating to the latest stable version is generally a good practice, it would be helpful to understand the motivation behind this specific update.

Are there any new features, bug fixes, or performance improvements in v27.3.1 that are particularly relevant to this project?

Has this version been tested to ensure compatibility with the project's current setup and dependencies?

Providing more context will help reviewers better understand the implications of this version update.

versions/K3S_VERSION (1)

1-1: K3S version updated to v1.31.1-k3s1.

The update to a new major version of K3S is noted. Please ensure that:

The K3S release notes for v1.31.1-k3s1 have been reviewed for any breaking changes or new features that may impact the project.

Thorough testing is performed to confirm that the project functions as expected with this new version of K3S.

.github/ISSUE_TEMPLATE/security_issue_report.md (1)

22-24: LGTM!

The updates to the Docker, Kubernetes, and Helm versions in the security issue report template are consistent with the PR objective of updating project dependencies. These changes do not affect any code or functionality and can be safely merged.

.github/ISSUE_TEMPLATE/bug_report.md (1)

28-30: LGTM!

The version updates for Docker, Kubernetes, and Helm are consistent with the PR objectives and are unlikely to introduce any breaking changes or impact the functionality of the bug report template.

k8s/external/minio/mb-job.yaml (1)

26-26: Verify the impact of the imagePullPolicy change.

The change of imagePullPolicy from Always to IfNotPresent can help reduce unnecessary image pulls and improve efficiency, especially in development or testing environments where the image is not frequently updated.

However, for production environments, consider using the Always policy to ensure the latest image is always pulled, especially if the image tag is mutable (e.g., latest). This can help maintain consistency across nodes and avoid potential issues caused by stale images.

Please ensure that the impact of this change is carefully verified and consider adjusting the policy based on the environment and image management strategy.

example/manifest/scylla/job.yaml (1)

26-26: Consider the implications of changing imagePullPolicy to IfNotPresent.

Changing the imagePullPolicy from Always to IfNotPresent can speed up pod startup times by skipping unnecessary image pulls when the image already exists on the node. However, it's important to consider the implications:

If the cassandra:latest image is frequently updated with important changes or security fixes, using IfNotPresent could result in pods running with outdated or vulnerable images.

If the image is stable and infrequently updated, IfNotPresent can provide performance benefits without significant drawbacks.

Please ensure that the cassandra:latest image is stable and infrequently updated before proceeding with this change. If the image is updated regularly, consider using a specific version tag instead of latest to have more control over updates.

k8s/external/minio/deployment.yaml (1)

36-36: Consider the trade-offs of using IfNotPresent for imagePullPolicy.

Changing imagePullPolicy to IfNotPresent is a valid configuration and can help reduce unnecessary image pulls, improving efficiency. However, please ensure that you have proper processes in place to manage and update the minio image version on the nodes. Using IfNotPresent means that if the image on the node becomes outdated, it will not be automatically updated, potentially leading to inconsistencies or missing important bug fixes or features.

To verify if this change aligns with your image management strategy, consider:

How often do you update the minio image version?

Do you have processes to ensure the desired version is propagated to all nodes?

Are there any auto-update mechanisms that could be affected by this change?

Reviewing these aspects will help confirm if IfNotPresent is suitable for your use case while mitigating any risks.

k8s/tools/cli/loadtest/job.yaml (1)

36-36: Approve the imagePullPolicy change with a caution.

The change aligns with the PR objective of updating imagePullPolicy from Always to IfNotPresent. This can speed up job deployments by avoiding unnecessary image pulls when the image already exists on the node.

However, be cautious about potential risks:

Using IfNotPresent with a nightly tag may result in running an outdated image if the tag is not regularly updated in the registry.

To mitigate this risk, consider:

Ensuring a process to regularly update the nightly tag in the registry.

Or, switching to a unique, versioned tag for each release to guarantee the intended version is run.
k8s/tools/cli/loadtest/cronjob.yaml (1)
40-40: Approve the change to imagePullPolicy with a caveat.

Changing the imagePullPolicy to IfNotPresent for a frequently running CronJob is a good optimization to reduce unnecessary image pulls and improve startup times.

However, please ensure that you have a process in place to update the image tag (vdaas/vald-loadtest:nightly in this case) whenever a new version of the image is pushed. Otherwise, the CronJob may continue using an outdated version of the image indefinitely.

To verify if the vald-loadtest image tag is being updated appropriately, you can run the following script:
#!/bin/bash
# Description: Verify the `vald-loadtest` image tag is being updated.

# Test: Search for the `vald-loadtest` image tag in the codebase. 
# Expect: The tag to be parameterized or updated in tandem with the image.
rg --type yaml $'vdaas/vald-loadtest:nightly'
example/client/go.mod (2)

17-17: Verify compatibility and test thoroughly.

The google.golang.org/grpc dependency has been updated from v1.66.2 to v1.67.0. This is a minor version update, which typically includes new features, bug fixes, and performance improvements while maintaining backward compatibility.

However, since gRPC is a core dependency for this project, it's important to:

Verify that the update does not introduce any breaking changes or compatibility issues.

Thoroughly test the application to ensure that all functionality works as expected with the updated dependency.

36-36: Verify compatibility and test thoroughly.

The golang.org/x/net dependency has been updated from v0.26.0 to v0.28.0. This is a minor version update, which typically includes new features, bug fixes, and performance improvements while maintaining backward compatibility.

However, since this package provides supplementary networking libraries, it's important to:

Verify that the update does not introduce any breaking changes or compatibility issues.

Thoroughly test the application to ensure that all functionality works as expected with the updated dependency.

dockers/agent/core/agent/Dockerfile (1)

43-43: LGTM!

The reordering of environment variable declarations improves readability without affecting functionality. The change is cosmetic and groups related variables together.

k8s/tools/benchmark/operator/deployment.yaml (1)

46-46: Verify if the imagePullPolicy change aligns with the desired update strategy.

Changing imagePullPolicy from Always to IfNotPresent can reduce unnecessary image pulls and improve pod startup time. However, please ensure this change aligns with your intended update strategy for the vald-benchmark-operator component.

If you always want the latest image to be pulled, then Always might be more suitable. If you prefer to use the locally cached image (if available) for efficiency and only pull when the image doesn't exist on the node, then IfNotPresent is the right choice.

internal/net/grpc/interceptor/server/logging/accesslog.go (3)

56-77: LGTM!

The new String() method is a great addition to the AccessLogEntity struct. It provides a convenient way to format the entity as a string, including error messages when applicable. This can be very helpful for logging and debugging purposes.

Some key benefits of this method:

It handles the case when there is an error associated with the entity and attempts to retrieve a status message from it using the status.FromError() function, which is a good practice for gRPC error handling.

It includes the error message in the string representation, making it easier to understand the context of the error.

It gracefully handles potential errors from JSON marshaling by providing a fallback string, ensuring that the method doesn't panic or return an empty string in case of marshaling failures.

Overall, this method enhances the logging capabilities of the AccessLogEntity and improves the overall maintainability and debuggability of the codebase.

118-118: Logging improvements in AccessLogInterceptor.

The updates to the logging statements in the AccessLogInterceptor function are excellent improvements:

Using the new String() method of AccessLogEntity in the logging statements enhances the readability and informativeness of the logs by including error messages when applicable. This makes it easier to understand the context of the logs, especially in case of errors.

Changing the log message for errors from "rpc completed" to "rpc failed" provides a more accurate description of the RPC outcome. This clarity helps in quickly identifying and understanding the nature of the logged event.

These changes are consistent with the new String() method and significantly improve the overall logging capabilities of the interceptor. They contribute to better maintainability, debuggability, and observability of the system.

Also applies to: 120-120

166-166: Logging improvements in AccessLogStreamInterceptor.

The updates to the logging statements in the AccessLogStreamInterceptor function are great enhancements:

Utilizing the new String() method of AccessLogEntity in the logging statements improves the readability and informativeness of the logs by including error messages when applicable. This provides better context and understanding of the logged events, particularly in error scenarios.

Updating the log message for errors from "rpc completed" to "rpc failed" offers a more precise description of the RPC outcome. This clarity facilitates quick identification and comprehension of the nature of the logged event.

These changes align with the new String() method and significantly enhance the overall logging capabilities of the interceptor. They promote better maintainability, debuggability, and observability of the system.

Also applies to: 168-168
internal/net/grpc/pool/pool_bench_test.go (2)
189-189: LGTM!

The change from grpc.DialContext to grpc.NewClient is consistent with the update made in the Benchmark_StaticDial function and aligns with the gRPC documentation's recommendation.

132-132: Verify grpc.NewClient usage consistency across the codebase.

The change from grpc.DialContext to grpc.NewClient aligns with the gRPC documentation's recommendation. Ensure that grpc.NewClient is being used consistently across the codebase to establish gRPC client connections.

Run the following script to verify the grpc.NewClient usage:
#!/bin/bash
# Description: Verify `grpc.NewClient` is used consistently to establish gRPC client connections.

# Test: Search for `grpc.DialContext` usage. Expect: No occurrences.
if rg --type go $'grpc\.DialContext'; then
  echo "FAIL: Found usage of deprecated grpc.DialContext"
  exit 1
fi

# Test: Search for `grpc.NewClient` usage. Expect: Only occurrences with 2 arguments - server address and dial options.
if rg --type go --line-number $'grpc\.NewClient' | awk '{print $2}' | grep -v '(.*,.*grpc\.WithTransportCredentials(.*))'; then 
  echo "FAIL: Found usage of grpc.NewClient with incorrect arguments"
  exit 1  
fi

echo "PASS: grpc.NewClient is being used consistently with the correct arguments"
internal/observability/exporter/otlp/otlp.go (1)

34-34: Approve the OpenTelemetry semantic conventions package version update.

The import statement for the OpenTelemetry semantic conventions package has been updated from version v1.24.0 to v1.26.0. This update ensures that the codebase is using the latest version of the library, which may include new semantic conventions, bug fixes, or improvements.

Please review the changelog or release notes of the OpenTelemetry library for any changes or deprecations in the semantic conventions between versions v1.24.0 and v1.26.0. Ensure that the codebase is thoroughly tested to confirm that the version update does not introduce any unexpected behavior or compatibility issues.

internal/observability/trace/status.go (1)

24-24: LGTM!

Updating the OpenTelemetry semantic conventions package to version v1.26.0 ensures that the latest attribute definitions are being used for gRPC status codes. This is a good practice to keep the dependencies up to date.

k8s/index/job/save/cronjob.yaml (2)

56-56: Verify that the required image version is present on the nodes.

Changing the imagePullPolicy to IfNotPresent can help optimize resource usage by avoiding unnecessary image pulls. However, please ensure that the required busybox:stable image version is already present on the nodes. If not, the pod may fail to start with this policy.

68-68: Verify that the required image version is present on the nodes.

This comment is a duplicate of the one made for the wait-for-agent initContainer. Please refer to that comment for more details.
k8s/index/job/creation/cronjob.yaml (3)
56-56: LGTM!

Setting imagePullPolicy to IfNotPresent for the wait-for-agent initContainer is a good optimization. It allows Kubernetes to use a locally cached image, if available, which can speed up pod startup times and reduce load on the image registry.

Just keep in mind that with IfNotPresent, if the image is updated in the registry, it won't be automatically pulled on existing nodes until the pods are restarted. So make sure to roll out the deployment if you update the image.

68-68: LGTM!

Similar to the change for the wait-for-agent initContainer, setting imagePullPolicy to IfNotPresent for the wait-for-discoverer initContainer is a good optimization.

Please refer to the previous comment for more context on the benefits and considerations of using IfNotPresent.

81-81: Verify image updates and pod restarts.

Changing imagePullPolicy from Always to IfNotPresent for the main vald-index-creation container aligns its behavior with the initContainers. This can speed up pod startup times and reduce load on the image registry.

However, with IfNotPresent, if you update the vald-index-creation image in the registry, the pods won't automatically use the new version until they are restarted. Make sure you have a process in place to roll out the deployment and restart pods when the image is updated.

To verify if there are any pods running with an outdated vald-index-creation image, you can run the following commands:
#!/bin/bash

# Get the current image version used in the cronjob
CURRENT_IMAGE=$(kubectl get cronjob vald-index-creation -o jsonpath='{.spec.jobTemplate.spec.template.spec.containers[?(@.name=="vald-index-creation")].image}')

# Get the image version running in the existing pods
PODS_IMAGE=$(kubectl get pods -l app=vald-index-creation -o jsonpath='{.items[*].spec.containers[?(@.name=="vald-index-creation")].image}')

# Compare the versions
if [ "$CURRENT_IMAGE" != "$PODS_IMAGE" ]; then
  echo "The pods are running an outdated vald-index-creation image."
  echo "Current image in cronjob: $CURRENT_IMAGE"
  echo "Image in running pods:    $PODS_IMAGE"
  echo "Please restart the pods to use the updated image."
else
  echo "The running pods are using the same vald-index-creation image version as the cronjob."
fi
This script compares the vald-index-creation image specified in the cronjob with the image running in the existing pods. If they differ, it means the pods are running an outdated version and should be restarted.
k8s/index/job/correction/cronjob.yaml (1)

56-56: LGTM! The IfNotPresent policy is a good choice for stable image tags.

Setting imagePullPolicy to IfNotPresent will skip the image pull if the image already exists on the node. This can speed up pod startup time and reduce unnecessary image pulls.

However, please ensure you have a robust image update strategy in place. With IfNotPresent, if the underlying image is updated, Kubernetes won't automatically pull the new version unless the pod template hash changes (e.g., by updating a label or annotation).

Consider using unique tags for each image version and updating the tag in the manifest whenever a new version should be deployed.

k8s/index/operator/deployment.yaml (2)

49-49: LGTM!

Updating the checksum/configmap annotation is a good practice to ensure the Deployment is updated when the referenced ConfigMap changes. This keeps the Deployment configuration in sync.

77-77: Verify if IfNotPresent aligns with your intended image update strategy.

Changing imagePullPolicy from Always to IfNotPresent can speed up deployments by reusing previously pulled images. However, consider the following tradeoffs:

Always ensures the latest image version is consistently used across all replicas, but increases startup time and network usage.

IfNotPresent improves startup time by reusing local images, but may result in inconsistent versions across replicas if the image is updated.

Choose the policy that best aligns with your desired balance of consistency, speed, and resource usage.
docs/user-guides/filtering-configuration.md (1)
156-156: Verify the usage of the custom grpc.NewClient function.

The code change replaces grpc.DialContext with a custom grpc.NewClient function for establishing the gRPC client connection. Ensure that this change is consistently applied across the codebase and that the behavior of the custom function is well-documented and understood.

To verify the usage of grpc.NewClient, run the following script:
#!/bin/bash
# Description: Verify the usage of the custom `grpc.NewClient` function across the codebase.

# Test 1: Search for occurrences of `grpc.NewClient`. Expect: Only valid usages.
rg --type go $'grpc\.NewClient'

# Test 2: Search for occurrences of `grpc.DialContext`. Expect: No remaining usages.
rg --type go $'grpc\.DialContext'
tests/e2e/performance/max_vector_dim_test.go (2)

Line range hint 128-156: LGTM!

The changes align with the gRPC documentation's recommendation to transition from grpc.DialContext to grpc.NewClient. The error handling for the grpc.NewClient call is appropriate, and repositioning the ctx variable does not introduce any functional issues.

156-156: LGTM!

Repositioning the ctx variable before the cli.Insert call does not introduce any functional issues and makes the context available for use in the subsequent gRPC calls.
k8s/gateway/gateway/mirror/deployment.yaml (1)
58-58: Approve, but use caution when updating the image version.

Setting imagePullPolicy to IfNotPresent can speed up pod startup times by skipping image pulls if the image already exists on the node. However, be cautious when updating the image version, as the old version will continue to be used until the pod is recreated.

To ensure the correct image version is running after an update, use the following command:
kubectl get pods -l app=vald-mirror-gateway -o jsonpath='{.items[*].spec.containers[*].image}'
Verify that the output matches the expected image version.
tests/e2e/operation/operation.go (6)
Line range hint 185-197: LGTM!

The change from grpc.DialContext to grpc.NewClient aligns with the removal of the ctx parameter. The implementation looks good.

199-206: LGTM!

The removal of the ctx parameter is fine, as the getGRPCConn method is handling the context internally. The implementation looks good.

Line range hint 208-215: LGTM!

The removal of the ctx parameter is fine, as the getGRPCConn method is handling the context internally. The implementation looks good.

Line range hint 153-161: Verify the context handling in getAgentClient.

The removal of the ctx parameter is fine, but ensure that the getAgentClient method is handling the context appropriately.

Run the following script to verify the getAgentClient method:
#!/bin/bash
# Description: Verify the context handling in `getAgentClient` method.

# Test: Search for the method implementation. Expect: The method should handle the context internally.
ast-grep --lang go --pattern $'func (c *client) getAgentClient() (core.AgentClient, error) {
  $$$
}'
Line range hint 166-174: Verify the context handling in getAgentClient.

The removal of the ctx parameter is fine, but ensure that the getAgentClient method is handling the context appropriately.

Run the following script to verify the getAgentClient method:
#!/bin/bash
# Description: Verify the context handling in `getAgentClient` method.

# Test: Search for the method implementation. Expect: The method should handle the context internally.
ast-grep --lang go --pattern $'func (c *client) getAgentClient() (core.AgentClient, error) {
  $$$
}'
177-183: Verify the context handling in getClient.

The removal of the ctx parameter is fine, but ensure that the getClient method is handling the context appropriately.

Run the following script to verify the getClient method:
#!/bin/bash
# Description: Verify the context handling in `getClient` method.

# Test: Search for the method implementation. Expect: The method should handle the context internally.
ast-grep --lang go --pattern $'func (c *client) getClient() (vald.Client, error) {
  $$$
}'
k8s/manager/index/deployment.yaml (3)

62-62: imagePullPolicy changed to IfNotPresent for the wait-for-agent init container.

This change optimizes pod startup time by avoiding unnecessary image pulls when the image already exists locally. However, please ensure the required image version is pre-pulled on the nodes, as updates to the image will not be automatically pulled with this policy.

74-74: imagePullPolicy changed to IfNotPresent for the wait-for-discoverer init container.

Looks good! The same considerations mentioned for the wait-for-agent init container apply here.

96-96: imagePullPolicy changed from Always to IfNotPresent for the main vald-manager-index container.

While this change can reduce unnecessary image pulls, please verify if this is the desired behavior for the main container in a production deployment. With IfNotPresent, any updates to the image tag in the deployment config will not be reflected until the existing pods are manually deleted. The previous Always policy would ensure the latest specified image version is pulled on each deployment update.

Consider if you want the image to be automatically updated on each deployment change or if using a stable, manually pre-pulled image is preferred. The latter case aligns better with the IfNotPresent policy.
k8s/gateway/gateway/lb/deployment.yaml (3)
61-61: Consider the image update frequency and version requirements.

Setting imagePullPolicy to IfNotPresent can speed up pod startup times by skipping unnecessary image pulls when the image already exists on the node. However, this also means that pods may start using an outdated cached version if the image tag is updated.

If the busybox:stable image is updated frequently and you always require the latest version, consider setting imagePullPolicy to Always instead. Otherwise, this change looks good.

73-73: Skipped providing feedback.

The same feedback about considering image update frequency and version requirements that was provided for the wait-for-discoverer init container also applies here.

104-104: Ensure you have a strategy to manage potential inconsistencies.

Changing imagePullPolicy from Always to IfNotPresent for the vald-lb-gateway container reduces unnecessary image pulls at the cost of potential inconsistencies if the image tag is updated but old versions are still cached on some nodes.

If you are using a unique image tag for each version (e.g. a semantic version or commit hash), this risk is mitigated since a new tag will force a new image pull. In that case, this change looks good to me.

However, if you are using a generic tag like latest or nightly, be cautious as this change means pods may continue running an outdated version even after the image is updated, until the old image is manually removed or evicted from each node's cache. Ensure you have a strategy to detect and manage such inconsistencies.

To verify if unique tags are being used for each image version, run:
#!/bin/bash
# Description: Check if unique image tags are being used for each version.

# Test: Search for the vald-lb-gateway image tag. Expect: A unique tag like a semantic version or commit hash.
rg --type yaml $'image:\s*"vdaas/vald-lb-gateway:' | rg --only-matching $':.*"$' | sort | uniq
tests/e2e/operation/multi.go (7)

27-27: Verify the impact of removing the ctx parameter.

The removal of the ctx parameter from the MultiSearch method signature and the getClient method call suggests that the context is no longer required for this operation. Please ensure that this change does not introduce any issues related to request cancellation, deadlines, or metadata propagation.

To verify the impact, you can perform the following steps:

Review the implementation of the getClient method to ensure that it does not rely on the context for any critical functionality.

Analyze the usage of the ctx parameter within the MultiSearch method and ensure that its removal does not affect any context-dependent operations.

Test the MultiSearch functionality thoroughly to validate that it behaves as expected without the context parameter.

If you encounter any issues or have concerns about the removal of the ctx parameter, please let me know, and I'll be happy to assist you further.

63-63: Verify the impact of removing the ctx parameter.

The removal of the ctx parameter from the MultiSearchByID method signature and the getClient method call suggests that the context is no longer required for this operation. Please ensure that this change does not introduce any issues related to request cancellation, deadlines, or metadata propagation.

To verify the impact, you can perform the following steps:

Review the implementation of the getClient method to ensure that it does not rely on the context for any critical functionality.

Analyze the usage of the ctx parameter within the MultiSearchByID method and ensure that its removal does not affect any context-dependent operations.

Test the MultiSearchByID functionality thoroughly to validate that it behaves as expected without the context parameter.

If you encounter any issues or have concerns about the removal of the ctx parameter, please let me know, and I'll be happy to assist you further.

99-99: Verify the impact of removing the ctx parameter.

The removal of the ctx parameter from the MultiLinearSearch method signature and the getClient method call suggests that the context is no longer required for this operation. Please ensure that this change does not introduce any issues related to request cancellation, deadlines, or metadata propagation.

To verify the impact, you can perform the following steps:

Review the implementation of the getClient method to ensure that it does not rely on the context for any critical functionality.

Analyze the usage of the ctx parameter within the MultiLinearSearch method and ensure that its removal does not affect any context-dependent operations.

Test the MultiLinearSearch functionality thoroughly to validate that it behaves as expected without the context parameter.

If you encounter any issues or have concerns about the removal of the ctx parameter, please let me know, and I'll be happy to assist you further.

133-133: Verify the impact of removing the ctx parameter.

The removal of the ctx parameter from the MultiLinearSearchByID method signature and the getClient method call suggests that the context is no longer required for this operation. Please ensure that this change does not introduce any issues related to request cancellation, deadlines, or metadata propagation.

To verify the impact, you can perform the following steps:

Review the implementation of the getClient method to ensure that it does not rely on the context for any critical functionality.

Analyze the usage of the ctx parameter within the MultiLinearSearchByID method and ensure that its removal does not affect any context-dependent operations.

Test the MultiLinearSearchByID functionality thoroughly to validate that it behaves as expected without the context parameter.

If you encounter any issues or have concerns about the removal of the ctx parameter, please let me know, and I'll be happy to assist you further.

167-167: Verify the impact of removing the ctx parameter.

The removal of the ctx parameter from the MultiInsert method signature and the getClient method call suggests that the context is no longer required for this operation. Please ensure that this change does not introduce any issues related to request cancellation, deadlines, or metadata propagation.

To verify the impact, you can perform the following steps:

Review the implementation of the getClient method to ensure that it does not rely on the context for any critical functionality.

Analyze the usage of the ctx parameter within the MultiInsert method and ensure that its removal does not affect any context-dependent operations.

Test the MultiInsert functionality thoroughly to validate that it behaves as expected without the context parameter.

If you encounter any issues or have concerns about the removal of the ctx parameter, please let me know, and I'll be happy to assist you further.

204-204: Verify the impact of removing the ctx parameter.

The removal of the ctx parameter from the MultiUpdate method signature and the getClient method call suggests that the context is no longer required for this operation. Please ensure that this change does not introduce any issues related to request cancellation, deadlines, or metadata propagation.

To verify the impact, you can perform the following steps:

Review the implementation of the getClient method to ensure that it does not rely on the context for any critical functionality.

Analyze the usage of the ctx parameter within the MultiUpdate method and ensure that its removal does not affect any context-dependent operations.

Test the MultiUpdate functionality thoroughly to validate that it behaves as expected without the context parameter.

If you encounter any issues or have concerns about the removal of the ctx parameter, please let me know, and I'll be happy to assist you further.

244-244: Verify the impact of removing the ctx parameter.

The removal of the ctx parameter from the MultiUpsert method signature and the getClient method call suggests that the context is no longer required for this operation. Please ensure that this change does not introduce any issues related to request cancellation, deadlines, or metadata propagation.

To verify the impact, you can perform the following steps:

Review the implementation of the getClient method to ensure that it does not rely on the context for any critical functionality.

Analyze the usage of the ctx parameter within the MultiUpsert method and ensure that its removal does not affect any context-dependent operations.

Test the MultiUpsert functionality thoroughly to validate that it behaves as expected without the context parameter.

If you encounter any issues or have concerns about the removal of the ctx parameter, please let me know, and I'll be happy to assist you further.

example/client/main.go (1)

69-69: LGTM! The change from grpc.DialContext to grpc.NewClient is a good improvement.

The grpc.NewClient method is a newer and recommended approach for creating gRPC clients. It offers a more idiomatic way of establishing connections to gRPC servers and might provide additional features or performance improvements compared to grpc.DialContext.

This change should not have any negative impact on the functionality, as the conn variable will still hold the established connection to the Vald cluster.

example/client/mirror/main.go (1)

71-71: Verify if the gRPC connection method change aligns with the use case requirements.

The change from grpc.DialContext to grpc.NewClient for establishing the gRPC connection aligns with the gRPC best practices and is approved.

However, please consider the following:

Evaluate if removing the context parameter impacts the ability to cancel or timeout the connection attempt, and if that aligns with the use case requirements.

Assess if the continued use of grpc.WithInsecure() without TLS is appropriate for the security requirements of this connection. If sensitive data is being transmitted, consider adding connection security.

example/client/agent/main.go (1)

70-70: Looks good! The change from grpc.DialContext to grpc.NewClient is the recommended approach.

As mentioned in the learning:

In grpc-go, grpc.DialContext has been deprecated and grpc.NewClient should be used instead.

This change updates the client creation to align with the current grpc-go recommendations. The error handling remains consistent.

internal/net/grpc/errdetails/errdetails.go (2)

61-74: LGTM!

The changes to export the constant names and explicitly convert the proto descriptor names to strings are good improvements for clarity and consistency.

237-318: LGTM!

The updates to the switch cases to use the new exported constant names are consistent with the earlier changes and look good.

docs/tutorial/get-started-with-faiss-agent.md (2)

281-281: LGTM!

The change from grpc.DialContext to grpc.NewClient looks good. It likely reflects an update to the recommended approach for creating gRPC client connections.

281-281: Skipping outdated comment.

The past review comment about the casing of "conn" is no longer applicable as the line has been modified.

docs/tutorial/vald-agent-standalone-on-k8s.md (1)

239-239: LGTM!

The change from grpc.DialContext to grpc.NewClient aligns with the gRPC recommendation to migrate away from the deprecated DialContext method. The parameters remain consistent, so this is a straightforward update.

docs/tutorial/get-started.md (1)

336-336: LGTM!

The change from grpc.DialContext to grpc.NewClient looks good. It likely reflects an update to align with the current recommended usage pattern in the gRPC library.

The past LanguageTool comment about capitalization does not apply here as this is a line of code, not a sentence in prose. Skipping regeneration of that comment.

docs/user-guides/client-api-config.md (5)

49-49: Verify the impact of removing the context from the connection method.

The change from grpc.DialContext to grpc.NewClient is consistent with the gRPC documentation. However, please ensure that removing the context from the connection method does not affect any scenarios where the connection needs to be cancelled or have a deadline.

165-165: Verify the impact of removing the context from the connection method.

The change from grpc.DialContext to grpc.NewClient is consistent with the gRPC documentation. However, please ensure that removing the context from the connection method does not affect any scenarios where the connection needs to be cancelled or have a deadline.

289-289: Verify the impact of removing the context from the connection method.

The change from grpc.DialContext to grpc.NewClient is consistent with the gRPC documentation. However, please ensure that removing the context from the connection method does not affect any scenarios where the connection needs to be cancelled or have a deadline.

473-473: Verify the impact of removing the context from the connection method.

The change from grpc.DialContext to grpc.NewClient is consistent with the gRPC documentation. However, please ensure that removing the context from the connection method does not affect any scenarios where the connection needs to be cancelled or have a deadline.

656-656: Verify the impact of removing the context from the connection method.

The change from grpc.DialContext to grpc.NewClient is consistent with the gRPC documentation. However, please ensure that removing the context from the connection method does not affect any scenarios where the connection needs to be cancelled or have a deadline.

internal/net/grpc/pool/pool.go (3)

Line range hint 130-146: LGTM!

The change from grpc.DialContext to grpc.NewClient aligns with the gRPC documentation and the updated code handles the connection establishment and error scenarios correctly.

Line range hint 472-492: LGTM!

The change from grpc.DialContext to grpc.NewClient aligns with the gRPC documentation. The updated code handles the connection establishment, health check, error scenarios, and retry with backoff correctly.

Line range hint 700-709: LGTM!

The change from grpc.DialContext to grpc.NewClient aligns with the gRPC documentation. The updated code scans for an available gRPC port, establishes a connection, and handles the success and error scenarios correctly.
cmd/index/operator/sample.yaml (4)
202-202: Verify the impact of changing imagePullPolicy to IfNotPresent.

Changing the imagePullPolicy to IfNotPresent can speed up pod startup times by avoiding unnecessary image pulls. However, ensure that the required image version will always be available on the node. If the image is accidentally deleted from the node, the pod will fail to start.

Run the following script to check if the image exists on all nodes:
#!/bin/bash
# Description: Verify the vald-readreplica-rotate image exists on all nodes.

# Test: Check if the image exists on all nodes. Expect: Image found on all nodes.
for node in $(kubectl get nodes -o jsonpath='{.items[*].metadata.name}'); do
  nodeimg=$(kubectl get node $node -o jsonpath="{.status.images[?(@.names[0]=='vdaas/vald-readreplica-rotate:pr-2444')].names[0]}")
  if [[ -z "$nodeimg" ]]; then
    echo "Image not found on node $node"
    exit 1
  fi  
done
echo "Image found on all nodes"
338-338: Verify the impact of changing imagePullPolicy to IfNotPresent.

Similar to the previous comment, ensure that the vald-index-creation image will always be available on the node. If the image is accidentally deleted, the job will fail.

Run the following script to check if the image exists on all nodes:
#!/bin/bash 
# Description: Verify the vald-index-creation image exists on all nodes.

# Test: Check if the image exists on all nodes. Expect: Image found on all nodes. 
for node in $(kubectl get nodes -o jsonpath='{.items[*].metadata.name}'); do
  nodeimg=$(kubectl get node $node -o jsonpath="{.status.images[?(@.names[0]=='vdaas/vald-index-creation:pr-2444')].names[0]}")
  if [[ -z "$nodeimg" ]]; then
    echo "Image not found on node $node"
    exit 1
  fi
done
echo "Image found on all nodes"  
453-453: Verify the impact of changing imagePullPolicy to IfNotPresent.

As with the other jobs, ensure that the vald-index-save image will always be available on the node. If the image is accidentally deleted, the job will fail.

Run the following script to check if the image exists on all nodes:
#!/bin/bash
# Description: Verify the vald-index-save image exists on all nodes.

# Test: Check if the image exists on all nodes. Expect: Image found on all nodes.
for node in $(kubectl get nodes -o jsonpath='{.items[*].metadata.name}'); do 
  nodeimg=$(kubectl get node $node -o jsonpath="{.status.images[?(@.names[0]=='vdaas/vald-index-save:pr-2444')].names[0]}")
  if [[ -z "$nodeimg" ]]; then
    echo "Image not found on node $node"
    exit 1
  fi
done
echo "Image found on all nodes"
568-568: Verify the impact of changing imagePullPolicy to IfNotPresent.

Consistent with the other jobs, ensure that the vald-index-correction image will always be available on the node. If the image is accidentally deleted, the job will fail.

Run the following script to check if the image exists on all nodes:
#!/bin/bash
# Description: Verify the vald-index-correction image exists on all nodes.

# Test: Check if the image exists on all nodes. Expect: Image found on all nodes.
for node in $(kubectl get nodes -o jsonpath='{.items[*].metadata.name}'); do
  nodeimg=$(kubectl get node $node -o jsonpath="{.status.images[?(@.names[0]=='vdaas/vald-index-correction:pr-2444')].names[0]}")
  if [[ -z "$nodeimg" ]]; then
    echo "Image not found on node $node" 
    exit 1
  fi
done
echo "Image found on all nodes"
tests/e2e/operation/stream.go (10)
96-96: Verify the removal of the context parameter.

The ctx parameter has been removed from the getClient method call. Please ensure that this change is intentional and aligns with the overall design and requirements of the system, especially regarding timeouts, cancellations, and deadlines.
#!/bin/bash
# Description: Verify if the `ctx` parameter is intentionally removed from `getClient` calls.

# Test: Search for `getClient` calls. Expect: No occurrences of `ctx` being passed.
rg --type go --glob '!stream.go' 'getClient\('
If the removal is intentional and does not introduce any issues, feel free to ignore this comment.

296-296: Verify the removal of the context parameter.

The ctx parameter has been removed from the getClient method call. Please ensure that this change is intentional and aligns with the overall design and requirements of the system, especially regarding timeouts, cancellations, and deadlines.
#!/bin/bash
# Description: Verify if the `ctx` parameter is intentionally removed from `getClient` calls.

# Test: Search for `getClient` calls. Expect: No occurrences of `ctx` being passed.
rg --type go --glob '!stream.go' 'getClient\('
If the removal is intentional and does not introduce any issues, feel free to ignore this comment.

416-416: Verify the removal of the context parameter.

The ctx parameter has been removed from the getClient method call. Please ensure that this change is intentional and aligns with the overall design and requirements of the system, especially regarding timeouts, cancellations, and deadlines.
#!/bin/bash
# Description: Verify if the `ctx` parameter is intentionally removed from `getClient` calls.

# Test: Search for `getClient` calls. Expect: No occurrences of `ctx` being passed.
rg --type go --glob '!stream.go' 'getClient\('
If the removal is intentional and does not introduce any issues, feel free to ignore this comment.

534-534: Verify the removal of the context parameter.

The ctx parameter has been removed from the getClient method call. Please ensure that this change is intentional and aligns with the overall design and requirements of the system, especially regarding timeouts, cancellations, and deadlines.
#!/bin/bash
# Description: Verify if the `ctx` parameter is intentionally removed from `getClient` calls.

# Test: Search for `getClient` calls. Expect: No occurrences of `ctx` being passed.
rg --type go --glob '!stream.go' 'getClient\('
If the removal is intentional and does not introduce any issues, feel free to ignore this comment.

640-640: Verify the removal of the context parameter.

The ctx parameter has been removed from the getClient method call. Please ensure that this change is intentional and aligns with the overall design and requirements of the system, especially regarding timeouts, cancellations, and deadlines.
#!/bin/bash
# Description: Verify if the `ctx` parameter is intentionally removed from `getClient` calls.

# Test: Search for `getClient` calls. Expect: No occurrences of `ctx` being passed.
rg --type go --glob '!stream.go' 'getClient\('
If the removal is intentional and does not introduce any issues, feel free to ignore this comment.

743-743: Verify the removal of the context parameter.

The ctx parameter has been removed from the getClient method call. Please ensure that this change is intentional and aligns with the overall design and requirements of the system, especially regarding timeouts, cancellations, and deadlines.
#!/bin/bash
# Description: Verify if the `ctx` parameter is intentionally removed from `getClient` calls.

# Test: Search for `getClient` calls. Expect: No occurrences of `ctx` being passed.
rg --type go --glob '!stream.go' 'getClient\('
If the removal is intentional and does not introduce any issues, feel free to ignore this comment.

847-847: Verify the removal of the context parameter.

The ctx parameter has been removed from the getClient method call. Please ensure that this change is intentional and aligns with the overall design and requirements of the system, especially regarding timeouts, cancellations, and deadlines.
#!/bin/bash
# Description: Verify if the `ctx` parameter is intentionally removed from `getClient` calls.

# Test: Search for `getClient` calls. Expect: No occurrences of `ctx` being passed.
rg --type go --glob '!stream.go' 'getClient\('
If the removal is intentional and does not introduce any issues, feel free to ignore this comment.

949-949: Verify the removal of the context parameter.

The ctx parameter has been removed from the getClient method call. Please ensure that this change is intentional and aligns with the overall design and requirements of the system, especially regarding timeouts, cancellations, and deadlines.
#!/bin/bash
# Description: Verify if the `ctx` parameter is intentionally removed from `getClient` calls.

# Test: Search for `getClient` calls. Expect: No occurrences of `ctx` being passed.
rg --type go --glob '!stream.go' 'getClient\('
If the removal is intentional and does not introduce any issues, feel free to ignore this comment.

1032-1032: Verify the removal of the context parameter.

The ctx parameter has been removed from the getClient method call. Please ensure that this change is intentional and aligns with the overall design and requirements of the system, especially regarding timeouts, cancellations, and deadlines.
#!/bin/bash
# Description: Verify if the `ctx` parameter is intentionally removed from `getClient` calls.

# Test: Search for `getClient` calls. Expect: No occurrences of `ctx` being passed.
rg --type go --glob '!stream.go' 'getClient\('
If the removal is intentional and does not introduce any issues, feel free to ignore this comment.

1050-1050: Verify the removal of the context parameter.

The ctx parameter has been removed from the getClient method call. Please ensure that this change is intentional and aligns with the overall design and requirements of the system, especially regarding timeouts, cancellations, and deadlines.
#!/bin/bash
# Description: Verify if the `ctx` parameter is intentionally removed from `getClient` calls.

# Test: Search for `getClient` calls. Expect: No occurrences of `ctx` being passed.
rg --type go --glob '!stream.go' 'getClient\('
If the removal is intentional and does not introduce any issues, feel free to ignore this comment.
hack/docker/gen/main.go (1)

417-417: LGTM!

The updated condition improves the deduplication logic by ensuring that only unique values are appended to the resulting map. This change prevents redundant data in the map values.

k8s/index/operator/configmap.yaml (3)

28-28: Approve the imagePullPolicy change for the vald-readreplica-rotate container.

The imagePullPolicy for the vald-readreplica-rotate container in the rotate job template has been appropriately changed from Always to IfNotPresent. This aligns with the PR objective and can help optimize image pulling by avoiding unnecessary pulls when the image already exists on the node.

28-28: Approve the imagePullPolicy changes for the vald-index-creation container and initContainers.

The imagePullPolicy for the vald-index-creation container and the initContainers (wait-for-agent and wait-for-discoverer) in the creation job template have been appropriately changed from Always to IfNotPresent. These changes align with the PR objective and can help optimize image pulling by avoiding unnecessary pulls when the images already exist on the node.

28-28: Approve the imagePullPolicy changes for the vald-index-save container and initContainers.

The imagePullPolicy for the vald-index-save container and the initContainers (wait-for-agent and wait-for-discoverer) in the save job template have been appropriately changed from Always to IfNotPresent. These changes align with the PR objective and can help optimize image pulling by avoiding unnecessary pulls when the images already exist on the node.

Makefile (2)

668-668: LGTM!

The formatting change to align the -DFAISS_ENABLE_GPU=OFF flag with other flags improves readability without introducing any functional changes.

682-703: Refactoring looks good!

The refactoring of the usearch installation script brings several improvements:

Using git clone and cmake provides more control and flexibility compared to downloading pre-built packages.

The cmake configuration allows enabling specific build options as needed.

The built libraries are correctly installed to the designated directories.

Temporary files are properly cleaned up after installation.

Overall, the refactoring enhances the maintainability and robustness of the usearch installation process without introducing any apparent issues.

internal/net/grpc/client.go (1)

168-168: LGTM!

Removing the grpc.WithBlock() option simplifies the connection logic by establishing connections asynchronously, which aligns with the best practice. The error handling logic remains intact to handle connection failures appropriately.

go.mod (20)

7-7: LGTM!

The minor version update for cloud.google.com/go/bigquery looks good. It should bring in bug fixes and improvements without introducing breaking changes.

8-8: LGTM!

The patch version update for cloud.google.com/go/compute looks good. It should only include backwards-compatible bug fixes.

11-11: LGTM!

The patch version update for cloud.google.com/go/iam looks good. It should only include backwards-compatible bug fixes.

12-12: LGTM!

The minor version update for cloud.google.com/go/kms looks good. It should bring in new features, bug fixes, and improvements without introducing breaking changes.

13-13: LGTM!

The patch version update for cloud.google.com/go/monitoring looks good. It should only include backwards-compatible bug fixes.

15-15: LGTM!

The patch version update for cloud.google.com/go/secretmanager looks good. It should only include backwards-compatible bug fixes.

17-17: LGTM!

The patch version update for cloud.google.com/go/trace looks good. It should only include backwards-compatible bug fixes.

48-48: LGTM!

The minor version update for github.com/aws/aws-sdk-go-v2 looks good. It should bring in new features, bug fixes, and improvements without introducing breaking changes.

49-49: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream looks good. It should only include backwards-compatible bug fixes.

50-50: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/config looks good. It should only include backwards-compatible bug fixes.

51-51: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/credentials looks good. It should only include backwards-compatible bug fixes.

52-52: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/feature/ec2/imds looks good. It should only include backwards-compatible bug fixes.

53-53: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/feature/s3/manager looks good. It should only include backwards-compatible bug fixes.

54-54: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/internal/configsources looks good. It should only include backwards-compatible bug fixes.

55-55: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 looks good. It should only include backwards-compatible bug fixes.

57-57: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding looks good. It should only include backwards-compatible bug fixes.

58-58: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/service/internal/checksum looks good. It should only include backwards-compatible bug fixes.

59-59: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/service/internal/presigned-url looks good. It should only include backwards-compatible bug fixes.

60-60: LGTM!

The patch version update for github.com/aws/aws-sdk-go-v2/service/internal/s3shared looks good. It should only include backwards-compatible bug fixes.

61-61: LGTM!

The minor version update for github.com/aws/aws-sdk-go-v2/service/kms looks good. It should bring in new features, bug fixes, and improvements without introducing breaking changes.

charts/vald/templates/_helpers.tpl (1)

702-702: LGTM!

The addition of the imagePullPolicy field in the initContainers configuration is a useful enhancement. It allows specifying the image pull policy for the init container, which dictates how the container image should be pulled (e.g., always, if not present, etc.).

The value of imagePullPolicy is dynamically set based on the .imagePullPolicy variable from the configuration context, providing flexibility in configuring the pull policy.

The conditional block ensures that the imagePullPolicy field is only included if the .type field is specified for the init container, maintaining backward compatibility and avoiding unexpected behavior.

charts/vald/values.yaml (5)

1078-1078: Looks good!

Setting imagePullPolicy: IfNotPresent for the wait container is a reasonable optimization to avoid unnecessary image pulls.

1084-1084: Looks good!

Consistently applying imagePullPolicy: IfNotPresent to the busybox-based wait containers is a good optimization.

1362-1362: Looks good!

Setting imagePullPolicy: IfNotPresent for the busybox wait container in the filter gateway is consistent with the changes made in the lb gateway.

1650-1650: Looks good!

The IfNotPresent pull policy is now consistently applied for busybox wait containers across all gateway components. This is a good optimization.

Line range hint 1-3800: Overall review: Approved!

The changes consistently apply imagePullPolicy: IfNotPresent to busybox-based wait containers across gateway and manager components. This optimization avoids unnecessary image pulls and speeds up pod startup.

The AI-generated summary accurately captures the essence of these changes. No inconsistencies found between the code changes and the summary.

Great work on consistently applying this best practice! The changes look good to merge.
.gitfiles (8)
170-171: Verify Generated Go Files for meta Service

The files meta.pb.go and meta_vtproto.pb.go are generated from meta.proto. Please ensure that these files are properly regenerated using the correct versions of protoc and any plugins to avoid compatibility issues.

889-891: Ensure Comprehensive Unit Tests for usearch Algorithm

The new usearch algorithm implementation has been added. Please verify that usearch_test.go covers all functionalities, including edge cases and error handling scenarios, to ensure the robustness of the algorithm.

1431-1434: Ensure Consistency in Discoverer Kubernetes Configurations

The daemonset.yaml, hpa.yaml, and networkpolicy.yaml for the discoverer component have been updated. Verify that these configurations are consistent with the overall deployment strategy and that resource requests and limits are appropriately set.

1985-1986: Validate Generated Rust Protobuf Files

The files meta.v1.rs and meta.v1.tonic.rs are generated from protobuf definitions. Confirm that these files are up-to-date and generated using the correct protobuf compiler versions to maintain compatibility.

1987-1987: [Duplicate Comment]

An existing review comment addresses potential issues in mirror.v1.rs. Please ensure that the previously highlighted concerns have been resolved.

226-226: Validate Swagger Documentation for meta API

The meta.swagger.json file defines the Swagger documentation for the new meta API. Ensure that all endpoints and data models are accurately represented and that the JSON adheres to Swagger specifications.
#!/bin/bash
# Description: Validate the Swagger JSON file for adherence to specifications.

# Install swagger-cli if not already installed.
npm install -g swagger-cli

# Validate the Swagger file.
swagger-cli validate apis/swagger/v1/meta/meta.swagger.json
1412-1424: Verify Kubernetes Configurations for agent Component

Several Kubernetes configuration files for the agent component have been updated. Please ensure the following:

Image Pull Policy: imagePullPolicy should be set to IfNotPresent instead of Always in deployments and initContainers, aligning with PR Objectives 🚨 Fix lint. #2 and [DepShield] (CVSS 5.9) Vulnerability due to usage of golang.org/x:crypto:0.0.0-20181203042331-505ab145d0a9 #6.

Configuration Validity: All YAML files should be validated for syntax errors and compliance with Kubernetes best practices.
#!/bin/bash
# Description: Verify that imagePullPolicy is correctly set in agent configurations.

# Check for imagePullPolicy not set to IfNotPresent.
rg --type yaml 'imagePullPolicy:\s*(?!IfNotPresent)' k8s/agent/

# Validate YAML files for syntax errors.
yamlfmt -d k8s/agent/
2045-2045: Integrate USEARCH_VERSION Consistently Across Build Scripts

A new version variable USEARCH_VERSION has been added. Ensure that:

Version Consistency: The version specified matches the intended USEARCH library version.

Build Integration: All build and deployment scripts reference USEARCH_VERSION where necessary.

Documentation: Update any relevant documentation to include instructions or notes about USEARCH_VERSION.
#!/bin/bash
# Description: Ensure USEARCH_VERSION is consistently used in build scripts.

# Search for USEARCH_VERSION usage in scripts.
rg 'USEARCH_VERSION' Makefile* scripts/ dockerfiles/

# Verify that there's no mismatch in version numbers.
rg 'USEARCH_VERSION\s*=\s*".+"' versions/ | awk -F'"' '{print $2}'

coderabbitai · 2024-09-21T06:46:24Z

k8s/index/job/save/cronjob.yaml

@@ -76,7 +78,7 @@ spec:
          containers:
            - name: vald-index-save
              image: "vdaas/vald-index-save:nightly"
-              imagePullPolicy: Always
+              imagePullPolicy: IfNotPresent


Consider using Always pull policy for the nightly image.

Since the vdaas/vald-index-save:nightly image is likely updated frequently, using the IfNotPresent pull policy may cause the pod to use an outdated version of the image. This could lead to unexpected behavior or compatibility issues.

Consider keeping the Always pull policy for the nightly image to ensure the latest version is pulled every time the pod is started.

k8s/index/job/correction/cronjob.yaml

internal/net/grpc/status/status.go

.gitfiles

Signed-off-by: kpango <[email protected]>

docs/tutorial/get-started-with-faiss-agent.md

docs/tutorial/get-started.md

docs/tutorial/vald-agent-standalone-on-k8s.md

coderabbitai

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (5)

docs/user-guides/client-api-config.md (5)

49-49: Approve code change and suggest formatting improvement

The update to grpc.NewClient(target) is correct and consistent with the PR objectives. However, there's a minor formatting issue:

Replace the hard tab at the beginning of the line with spaces to adhere to Markdown best practices and maintain consistency throughout the document.

🧰 Tools

🪛 Markdownlint

49-49: Column: 1
Hard tabs

(MD010, no-hard-tabs)

165-165: Approve code change and suggest formatting improvement

The update to grpc.NewClient(target) is correct and consistent with the previous changes and PR objectives.

Replace the hard tab at the beginning of the line with spaces to adhere to Markdown best practices and maintain consistency throughout the document.

🧰 Tools

🪛 Markdownlint

165-165: Column: 1
Hard tabs

(MD010, no-hard-tabs)

289-289: Approve code change and suggest formatting improvement

The update to grpc.NewClient(target) is correct and consistent with the previous changes and PR objectives.

Replace the hard tab at the beginning of the line with spaces to adhere to Markdown best practices and maintain consistency throughout the document.

🧰 Tools

🪛 Markdownlint

289-289: Column: 1
Hard tabs

(MD010, no-hard-tabs)

473-473: Approve code change and suggest formatting improvement

The update to grpc.NewClient(target) is correct and consistent with the previous changes and PR objectives.

Replace the hard tab at the beginning of the line with spaces to adhere to Markdown best practices and maintain consistency throughout the document.

🧰 Tools

🪛 Markdownlint

473-473: Column: 1
Hard tabs

(MD010, no-hard-tabs)

656-656: Approve code change, suggest formatting improvement, and provide overall feedback

The update to grpc.NewClient(target) is correct and consistent with the previous changes and PR objectives.

Replace the hard tab at the beginning of the line with spaces to adhere to Markdown best practices and maintain consistency throughout the document.

Overall, the changes to replace grpc.DialContext with grpc.NewClient have been consistently applied throughout the document. This update aligns well with the PR objectives and improves the codebase by using the recommended gRPC connection method.

After making these formatting changes, please review the surrounding documentation to ensure it accurately reflects the new grpc.NewClient method. If there are any explanations or comments about the connection process, they may need to be updated as well.

🧰 Tools

🪛 Markdownlint

656-656: Column: 1
Hard tabs

(MD010, no-hard-tabs)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 0a14610 and 9ff43ed.

⛔ Files ignored due to path filters (5)

apis/grpc/v1/agent/sidecar/sidecar_vtproto.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
example/client/go.sum is excluded by !**/*.sum
go.sum is excluded by !**/*.sum
hack/docker/gen/main.go is excluded by !**/gen/**
rust/Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (72)

.gitfiles (9 hunks)
.github/ISSUE_TEMPLATE/bug_report.md (1 hunks)
.github/ISSUE_TEMPLATE/security_issue_report.md (1 hunks)
.github/PULL_REQUEST_TEMPLATE.md (3 hunks)
.github/workflows/e2e-profiling.yaml (4 hunks)
.github/workflows/e2e.yaml (1 hunks)
Makefile (2 hunks)
charts/vald/templates/_helpers.tpl (1 hunks)
charts/vald/values.yaml (7 hunks)
dockers/agent/core/agent/Dockerfile (1 hunks)
dockers/ci/base/Dockerfile (1 hunks)
dockers/dev/Dockerfile (1 hunks)
docs/tutorial/get-started-with-faiss-agent.md (1 hunks)
docs/tutorial/get-started.md (1 hunks)
docs/tutorial/vald-agent-standalone-on-k8s.md (1 hunks)
docs/user-guides/client-api-config.md (5 hunks)
docs/user-guides/filtering-configuration.md (1 hunks)
example/client/agent/main.go (1 hunks)
example/client/go.mod (2 hunks)
example/client/main.go (1 hunks)
example/client/mirror/main.go (1 hunks)
go.mod (13 hunks)
internal/core/algorithm/usearch/usearch_test.go (1 hunks)
internal/net/dialer.go (1 hunks)
internal/net/grpc/client.go (1 hunks)
internal/net/grpc/codes/codes.go (1 hunks)
internal/net/grpc/errdetails/errdetails.go (2 hunks)
internal/net/grpc/interceptor/server/logging/accesslog.go (5 hunks)
internal/net/grpc/pool/pool.go (4 hunks)
internal/net/grpc/pool/pool_bench_test.go (2 hunks)
internal/net/grpc/status/status.go (4 hunks)
internal/net/net.go (1 hunks)
internal/observability/exporter/otlp/otlp.go (1 hunks)
internal/observability/trace/status.go (1 hunks)
k8s/gateway/gateway/ing.yaml (1 hunks)
k8s/gateway/gateway/lb/deployment.yaml (2 hunks)
k8s/gateway/gateway/mirror/deployment.yaml (1 hunks)
k8s/index/job/correction/cronjob.yaml (2 hunks)
k8s/index/job/creation/cronjob.yaml (2 hunks)
k8s/index/job/save/cronjob.yaml (2 hunks)
k8s/index/operator/configmap.yaml (1 hunks)
k8s/index/operator/deployment.yaml (1 hunks)
k8s/manager/index/deployment.yaml (2 hunks)
k8s/metrics/loki/loki.yaml (1 hunks)
k8s/metrics/loki/promtail.yaml (1 hunks)
k8s/metrics/profefe/cronjob.yaml (1 hunks)
k8s/metrics/pyroscope/base/deployment.yaml (1 hunks)
k8s/metrics/tempo/tempo.yaml (2 hunks)
pkg/gateway/lb/handler/grpc/handler.go (1 hunks)
pkg/index/job/save/service/indexer_test.go (1 hunks)
tests/e2e/crud/crud_faiss_test.go (2 hunks)
tests/e2e/crud/crud_test.go (9 hunks)
tests/e2e/operation/multi.go (8 hunks)
tests/e2e/operation/operation.go (4 hunks)
tests/e2e/operation/stream.go (14 hunks)
tests/e2e/performance/max_vector_dim_test.go (2 hunks)
versions/CHAOS_MESH_VERSION (1 hunks)
versions/DOCKER_VERSION (1 hunks)
versions/HELM_VERSION (1 hunks)
versions/KUBECTL_VERSION (1 hunks)
versions/PROMETHEUS_STACK_VERSION (1 hunks)
versions/PROTOBUF_VERSION (1 hunks)
versions/REVIEWDOG_VERSION (1 hunks)
versions/actions/ACTIONS_CHECKOUT (1 hunks)
versions/actions/ACTIONS_SETUP_NODE (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_ANALYZE (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_INIT (1 hunks)
versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF (1 hunks)
versions/actions/GITHUB_ISSUE_METRICS (1 hunks)
versions/actions/PETER_EVANS_CREATE_ISSUE_FROM_FILE (1 hunks)
versions/actions/PETER_EVANS_CREATE_PULL_REQUEST (1 hunks)

✅ Files skipped from review due to trivial changes (5)

.github/PULL_REQUEST_TEMPLATE.md
versions/KUBECTL_VERSION
versions/PROTOBUF_VERSION
versions/actions/ACTIONS_SETUP_NODE
versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD

🚧 Files skipped from review as they are similar to previous changes (65)

.gitfiles
.github/ISSUE_TEMPLATE/bug_report.md
.github/ISSUE_TEMPLATE/security_issue_report.md
.github/workflows/e2e-profiling.yaml
.github/workflows/e2e.yaml
Makefile
charts/vald/templates/_helpers.tpl
charts/vald/values.yaml
dockers/agent/core/agent/Dockerfile
dockers/ci/base/Dockerfile
dockers/dev/Dockerfile
docs/tutorial/get-started-with-faiss-agent.md
docs/tutorial/get-started.md
docs/tutorial/vald-agent-standalone-on-k8s.md
example/client/agent/main.go
example/client/go.mod
example/client/main.go
example/client/mirror/main.go
go.mod
internal/core/algorithm/usearch/usearch_test.go
internal/net/dialer.go
internal/net/grpc/client.go
internal/net/grpc/codes/codes.go
internal/net/grpc/errdetails/errdetails.go
internal/net/grpc/interceptor/server/logging/accesslog.go
internal/net/grpc/pool/pool.go
internal/net/grpc/pool/pool_bench_test.go
internal/net/grpc/status/status.go
internal/net/net.go
internal/observability/exporter/otlp/otlp.go
internal/observability/trace/status.go
k8s/gateway/gateway/ing.yaml
k8s/gateway/gateway/lb/deployment.yaml
k8s/gateway/gateway/mirror/deployment.yaml
k8s/index/job/correction/cronjob.yaml
k8s/index/job/creation/cronjob.yaml
k8s/index/job/save/cronjob.yaml
k8s/index/operator/configmap.yaml
k8s/index/operator/deployment.yaml
k8s/manager/index/deployment.yaml
k8s/metrics/loki/loki.yaml
k8s/metrics/loki/promtail.yaml
k8s/metrics/profefe/cronjob.yaml
k8s/metrics/pyroscope/base/deployment.yaml
k8s/metrics/tempo/tempo.yaml
pkg/gateway/lb/handler/grpc/handler.go
pkg/index/job/save/service/indexer_test.go
tests/e2e/crud/crud_faiss_test.go
tests/e2e/crud/crud_test.go
tests/e2e/operation/multi.go
tests/e2e/operation/operation.go
tests/e2e/operation/stream.go
tests/e2e/performance/max_vector_dim_test.go
versions/CHAOS_MESH_VERSION
versions/DOCKER_VERSION
versions/HELM_VERSION
versions/PROMETHEUS_STACK_VERSION
versions/REVIEWDOG_VERSION
versions/actions/ACTIONS_CHECKOUT
versions/actions/GITHUB_CODEQL_ACTION_ANALYZE
versions/actions/GITHUB_CODEQL_ACTION_INIT
versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF
versions/actions/GITHUB_ISSUE_METRICS
versions/actions/PETER_EVANS_CREATE_ISSUE_FROM_FILE
versions/actions/PETER_EVANS_CREATE_PULL_REQUEST

🧰 Additional context used

🪛 Markdownlint

docs/user-guides/client-api-config.md

49-49: Column: 1
Hard tabs

(MD010, no-hard-tabs)

165-165: Column: 1
Hard tabs

(MD010, no-hard-tabs)

289-289: Column: 1
Hard tabs

(MD010, no-hard-tabs)

473-473: Column: 1
Hard tabs

(MD010, no-hard-tabs)

656-656: Column: 1
Hard tabs

(MD010, no-hard-tabs)

docs/user-guides/filtering-configuration.md

156-156: Column: 1
Hard tabs

(MD010, no-hard-tabs)

docs/user-guides/filtering-configuration.md

vdaas-ci · 2024-09-30T04:40:22Z

Profile Report

type	vald-agent-ngt	vald-lb-gateway	vald-discoverer	vald-manager-index
cpu
heap

other images

hlts2

LGTM!

ignore

Signed-off-by: kpango <[email protected]>

Signed-off-by: kpango <[email protected]> Co-authored-by: Yusuke Kato <[email protected]>

coderabbitai bot approved these changes Sep 21, 2024

View reviewed changes

kpango requested review from a team, kmrmt and hlts2 and removed request for a team September 21, 2024 06:23

coderabbitai bot requested changes Sep 21, 2024

View reviewed changes

kpango force-pushed the refactor/main/for-release-v1.7.14 branch from 3bea900 to a301e1a Compare September 23, 2024 06:38

kpango added actions/e2e-deploy actions/e2e-chaos actions/e2e-profiling actions/e2e-max-dim and removed actions/e2e-chaos actions/e2e-profiling actions/e2e-max-dim labels Sep 27, 2024

Refctor for release v1.7.14

9ff43ed

Signed-off-by: kpango <[email protected]>

kpango force-pushed the refactor/main/for-release-v1.7.14 branch from 0a14610 to 9ff43ed Compare September 27, 2024 21:54

github-actions bot reviewed Sep 27, 2024

View reviewed changes

docs/tutorial/get-started-with-faiss-agent.md Show resolved Hide resolved

docs/tutorial/get-started.md Show resolved Hide resolved

docs/tutorial/vald-agent-standalone-on-k8s.md Show resolved Hide resolved

coderabbitai bot previously requested changes Sep 27, 2024

View reviewed changes

docs/user-guides/filtering-configuration.md Show resolved Hide resolved

kpango added actions/e2e-deploy actions/e2e-chaos actions/e2e-profiling actions/e2e-max-dim and removed actions/e2e-deploy actions/e2e-chaos actions/e2e-profiling actions/e2e-max-dim labels Sep 30, 2024

kpango requested review from kmrmt and hlts2 September 30, 2024 04:50

hlts2 approved these changes Sep 30, 2024

View reviewed changes

kmrmt approved these changes Sep 30, 2024

View reviewed changes

kpango merged commit 51283dc into main Sep 30, 2024
309 of 313 checks passed

kpango deleted the refactor/main/for-release-v1.7.14 branch September 30, 2024 06:02

vdaas-ci pushed a commit that referenced this pull request Sep 30, 2024

Refctor for release v1.7.14 (#2639)

a76f96c

Signed-off-by: kpango <[email protected]>

coderabbitai bot mentioned this pull request Sep 30, 2024

Backport PR #2639 to release/v1.7 for Refctor for release v1.7.14 #2648

Merged

2 tasks

kpango added a commit that referenced this pull request Sep 30, 2024

Refctor for release v1.7.14 (#2639) (#2648)

6a1d008

Signed-off-by: kpango <[email protected]> Co-authored-by: Yusuke Kato <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refctor for release v1.7.14 #2639

Refctor for release v1.7.14 #2639

kpango commented Sep 21, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Sep 21, 2024 •

edited

Loading

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

cloudflare-workers-and-pages bot commented Sep 21, 2024 •

edited

Loading

codecov bot commented Sep 21, 2024 •

edited

Loading

coderabbitai bot left a comment

coderabbitai bot Sep 21, 2024

coderabbitai bot Sep 21, 2024

coderabbitai bot left a comment

coderabbitai bot Sep 21, 2024

coderabbitai bot Sep 21, 2024

coderabbitai bot Sep 21, 2024

coderabbitai bot left a comment

coderabbitai bot Sep 21, 2024

coderabbitai bot left a comment

vdaas-ci commented Sep 30, 2024

hlts2 left a comment

Refctor for release v1.7.14 #2639

Refctor for release v1.7.14 #2639

Conversation

kpango commented Sep 21, 2024 • edited by coderabbitai bot Loading

Description

Related Issue

Versions

Checklist

Special notes for your reviewer

Summary by CodeRabbit

Summary by CodeRabbit

coderabbitai bot commented Sep 21, 2024 • edited Loading

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

cloudflare-workers-and-pages bot commented Sep 21, 2024 • edited Loading

Deploying vald with Cloudflare Pages

codecov bot commented Sep 21, 2024 • edited Loading

Codecov Report

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Sep 21, 2024

Choose a reason for hiding this comment

coderabbitai bot Sep 21, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Sep 21, 2024

Choose a reason for hiding this comment

coderabbitai bot Sep 21, 2024

Choose a reason for hiding this comment

coderabbitai bot Sep 21, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Sep 21, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

vdaas-ci commented Sep 30, 2024

Profile Report

hlts2 left a comment

Choose a reason for hiding this comment

kpango commented Sep 21, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Sep 21, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

cloudflare-workers-and-pages bot commented Sep 21, 2024 •

edited

Loading

codecov bot commented Sep 21, 2024 •

edited

Loading