Backport PR #2778 to release/v1.7 for Fix format of proto files #2783
Conversation
Signed-off-by: vankichi <[email protected]>
Deploying vald with
|
| Latest commit: |
5f8aca7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://922f6a76.vald.pages.dev |
| Branch Preview URL: | https://backport-release-v1-7-refact-vjnl.vald.pages.dev |
📝 WalkthroughWalkthroughThis pull request focuses on documentation improvements across multiple Protobuf files in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[CHATOPS:HELP] ChatOps commands.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apis/proto/v1/vald/index.proto (1)
Line range hint
1-62: Note regarding static analysis suggestionsThe static analysis tool suggests using more specific request/response message types instead of
Empty. However, since:
- This is a backport PR
- The use of
Emptymessage is an established pattern in this codebase- Changing message types would be a breaking change
We should maintain consistency with the existing codebase structure.
🧰 Tools
🪛 buf (1.47.2)
34-36: "payload.v1.Empty" is used as the request or response type for multiple RPCs.
(RPC_REQUEST_RESPONSE_UNIQUE)
34-34: RPC request type "Empty" should be named "IndexInfoRequest" or "IndexIndexInfoRequest".
(RPC_REQUEST_STANDARD_NAME)
34-34: RPC response type "Count" should be named "IndexInfoResponse" or "IndexIndexInfoResponse".
(RPC_RESPONSE_STANDARD_NAME)
40-42: "payload.v1.Empty" is used as the request or response type for multiple RPCs.
(RPC_REQUEST_RESPONSE_UNIQUE)
40-40: RPC request type "Empty" should be named "IndexDetailRequest" or "IndexIndexDetailRequest".
(RPC_REQUEST_STANDARD_NAME)
40-40: RPC response type "Detail" should be named "IndexDetailResponse" or "IndexIndexDetailResponse".
(RPC_RESPONSE_STANDARD_NAME)
46-46: RPC request type "Empty" should be named "IndexStatisticsRequest" or "IndexIndexStatisticsRequest".
(RPC_REQUEST_STANDARD_NAME)
46-46: RPC response type "Statistics" should be named "IndexStatisticsResponse" or "IndexIndexStatisticsResponse".
(RPC_RESPONSE_STANDARD_NAME)
apis/proto/v1/vald/update.proto (1)
Line range hint
137-146: TODO placeholders need to be addressed in UpdateTimestamp method.The Status Code and Troubleshooting sections contain TODO placeholders. This affects the completeness of the API documentation.
Would you like me to help generate the missing documentation sections for the UpdateTimestamp method? I can create a new GitHub issue to track this task.
apis/proto/v1/vald/search.proto (1)
Line range hint
387-387: Remove extra forward slash in comment.There's an extra forward slash in the comment that could affect documentation rendering.
- // // --- + // ---apis/proto/v1/vald/remove.proto (2)
Line range hint
65-89: Fix table formatting in troubleshooting section.The table formatting is inconsistent in line 89. The closing cell marker is missing.
Apply this fix:
- // | INTERNAL | Target Vald cluster or network route has some critical error. | Check target Vald cluster first and check network route including ingress as second. + // | INTERNAL | Target Vald cluster or network route has some critical error. | Check target Vald cluster first and check network route including ingress as second. |🧰 Tools
🪛 buf (1.47.2)
90-90: RPC request type "TimestampRequest" should be named "RemoveByTimestampRequest" or "RemoveRemoveByTimestampRequest".
(RPC_REQUEST_STANDARD_NAME)
90-90: RPC response type "Locations" should be named "RemoveByTimestampResponse" or "RemoveRemoveByTimestampResponse".
(RPC_RESPONSE_STANDARD_NAME)
Line range hint
130-149: Consider enhancing the message size limitation notice.While the notice about gRPC message size limitations is valuable, it could be more helpful by including specific details.
Consider expanding the notice with:
- The default message size limit
- How to configure custom limits
- Best practices for handling large batch operations
Example enhancement:
// <div class="notice"> // gRPC has a message size limitation.<br> + // The default maximum message size is 4MB.<br> + // For larger batches, consider using StreamRemove instead or configure custom limits using:<br> + // - Server-side: grpc.MaxRecvMsgSize()<br> + // - Client-side: grpc.WithMaxMsgSize()<br> // Please be careful that the size of the request exceeds the limit. // </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apis/proto/v1/vald/filter.proto(5 hunks)apis/proto/v1/vald/index.proto(1 hunks)apis/proto/v1/vald/insert.proto(4 hunks)apis/proto/v1/vald/object.proto(3 hunks)apis/proto/v1/vald/remove.proto(7 hunks)apis/proto/v1/vald/search.proto(16 hunks)apis/proto/v1/vald/update.proto(4 hunks)apis/proto/v1/vald/upsert.proto(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- apis/proto/v1/vald/object.proto
- apis/proto/v1/vald/filter.proto
🧰 Additional context used
🪛 buf (1.47.2)
apis/proto/v1/vald/index.proto
40-42: "payload.v1.Empty" is used as the request or response type for multiple RPCs.
(RPC_REQUEST_RESPONSE_UNIQUE)
40-40: RPC request type "Empty" should be named "IndexDetailRequest" or "IndexIndexDetailRequest".
(RPC_REQUEST_STANDARD_NAME)
40-40: RPC response type "Detail" should be named "IndexDetailResponse" or "IndexIndexDetailResponse".
(RPC_RESPONSE_STANDARD_NAME)
🔇 Additional comments (10)
apis/proto/v1/vald/index.proto (1)
37-37: LGTM! Consistent spacing between method definitions.
The removal of extra blank lines improves the consistency of the proto file formatting.
Also applies to: 43-43
apis/proto/v1/vald/insert.proto (2)
Line range hint 47-56: LGTM! Well-structured troubleshooting documentation.
The table format provides clear and actionable guidance for error resolution.
Line range hint 93-97: LGTM! Important notice about gRPC message size limitations.
The HTML notice appropriately warns users about potential message size constraints in MultiInsert operations.
apis/proto/v1/vald/upsert.proto (1)
Line range hint 96-101: LGTM! Consistent documentation style.
The HTML notice about gRPC limitations matches the format used in insert.proto, maintaining consistency across services.
apis/proto/v1/vald/update.proto (1)
Line range hint 98-103: LGTM! Consistent documentation style.
The HTML notice about gRPC limitations matches the format used in other proto files, maintaining consistency across services.
apis/proto/v1/vald/search.proto (3)
46-48: LGTM! Well-structured troubleshooting documentation.
The added troubleshooting sections provide clear, consistent, and valuable information about common error scenarios and their resolutions across all RPC methods. The documentation will help developers quickly identify and resolve issues.
Also applies to: 78-80, 111-113, 139-141, 170-172, 206-208, 237-239, 270-272, 303-305, 331-333, 362-364, 398-400
Line range hint 153-157: LGTM! Important warning about gRPC message size limitations.
The added notices appropriately warn users about gRPC message size limitations for multi-request methods. This is crucial for preventing runtime issues when dealing with large batches of vectors.
Also applies to: 189-193, 345-349, 381-385
Line range hint 50-54: LGTM! Accurate and comprehensive error documentation.
The error descriptions and resolution steps are technically accurate, well-differentiated between vector-based and ID-based operations, and provide practical guidance for troubleshooting.
Also applies to: 82-86, 115-119, 143-147, 174-178, 210-214, 241-245, 274-278, 307-311, 335-339, 366-370, 402-406
apis/proto/v1/vald/remove.proto (2)
46-48: LGTM! Well-structured troubleshooting documentation.
The added troubleshooting section provides clear and actionable guidance for handling common error scenarios.
Line range hint 99-118: LGTM! Comprehensive streaming documentation.
The added documentation effectively explains the bidirectional streaming behavior and includes helpful links to gRPC documentation.
vankichi
deleted the
backport/release/v1.7/refactor/apis/fix-api-proto-format
branch
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Filter,Index,Insert,Object,Remove,Search,Update, andUpsert.Insert,Remove, andUpsertservices.