fix: wrong frontend registration address #4199
Conversation
|
Warning Rate limit exceeded@killme2008 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 46 minutes and 59 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce a function Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Datanode
participant Frontend
participant Servers as Servers::Addrs
Client->>+Datanode: Sends request with server name
Datanode->>+Servers: Resolve address using `resolve_addr`
Servers-->>-Datanode: Returns `peer_addr`
Datanode->>Frontend: Communicates with `peer_addr`
Frontend->>Frontend: Uses resolved address
Frontend-->>Client: Response with resolved peer information
Poem
🐇✨ 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/datanode/src/heartbeat.rs (6 hunks)
- src/frontend/src/heartbeat.rs (4 hunks)
- src/servers/src/addrs.rs (1 hunks)
- src/servers/src/lib.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- src/servers/src/lib.rs
Additional comments not posted (6)
src/frontend/src/heartbeat.rs (3)
25-25: Import ofservers::addrsis appropriate.This import is required for the updated address resolution logic in
HeartbeatTask.
41-41: Correct update ofpeer_addrinHeartbeatTask.The update to initialize
peer_addrusing theresolve_addrfunction from theaddrsmodule is correctly implemented and aligns with the PR's objectives.Also applies to: 57-57
57-57: Initialization ofHeartbeatTaskis correct.The
newmethod correctly initializes theHeartbeatTaskwith the updatedpeer_addr. The rest of the method's logic is appropriate and unchanged.src/datanode/src/heartbeat.rs (3)
31-31: Import ofservers::addrsis appropriate for the datanode.This import is required for the updated address resolution logic in
HeartbeatTask.
51-51: Correct update ofpeer_addrinHeartbeatTask.The update to initialize
peer_addrusing theresolve_addrfunction from theaddrsmodule is correctly implemented and aligns with the PR's objectives.Also applies to: 87-87
87-87: Initialization ofHeartbeatTaskin datanode is correct.The
try_newmethod correctly initializes theHeartbeatTaskwith the updatedpeer_addr. The rest of the method's logic is appropriate and unchanged.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4199 +/- ##
==========================================
- Coverage 85.05% 84.56% -0.49%
==========================================
Files 1031 1041 +10
Lines 181276 183023 +1747
==========================================
+ Hits 154176 154772 +596
- Misses 27100 28251 +1151 |
As I said in #4186 (comment) I think using gRPC is good currently, and the gRPC is the main communication between nodes, it's part of cluster-info. |
|
Flow's heartbeat task also need to do the same thing: greptimedb/src/flow/src/heartbeat.rs Line 38 in a44fe62 |
killme2008
force-pushed
the
feature/fix-endpoints
branch
from
064dcdd
to
d059e91
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- config/datanode.example.toml (1 hunks)
- config/frontend.example.toml (1 hunks)
- src/datanode/src/heartbeat.rs (6 hunks)
- src/frontend/src/heartbeat.rs (4 hunks)
- src/servers/src/addrs.rs (1 hunks)
- src/servers/src/lib.rs (1 hunks)
Files skipped from review due to trivial changes (2)
- config/datanode.example.toml
- config/frontend.example.toml
Files skipped from review as they are similar to previous changes (3)
- src/frontend/src/heartbeat.rs
- src/servers/src/addrs.rs
- src/servers/src/lib.rs
Additional comments not posted (1)
src/datanode/src/heartbeat.rs (1)
51-51: Validation of thepeer_addrinitialization.The
peer_addrfield in theHeartbeatTaskstruct is now initialized using theaddrs::resolve_addrfunction. This change aligns with the PR's objective to ensure correct peer addresses are registered. However, ensure that theresolve_addrfunction is robust and handles edge cases effectively, especially since it involves network configurations which can be tricky.It would be beneficial to verify that this function has comprehensive unit tests covering various scenarios, including error cases and boundary conditions.
Also applies to: 87-87
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- config/config.md (2 hunks)
Additional context used
LanguageTool
config/config.md
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | |mysql| -- | -- | MySQL server o...
[uncategorized] ~40-~40: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | |postgres| -- | -- | PostgresSQL ...
[uncategorized] ~49-~49: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | |opentsdb| -- | -- | OpenTSDB pro...
[grammar] ~93-~93: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...e| String |None` | The scope of the google cloud storage.
**It's only used whe...
[grammar] ~94-~94: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...g |None| The credential path of the google cloud storage.
**It's only used whe...
[style] ~134-~134: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | |...
[grammar] ~143-~143: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...Forstandalonemode,self_importis recommend to collect metrics generated by itself ...
[uncategorized] ~179-~179: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...
[uncategorized] ~179-~179: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | |mysql| -- | -- | MySQL server o...
[uncategorized] ~188-~188: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | |postgres| -- | -- | PostgresSQL ...
[uncategorized] ~197-~197: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | |opentsdb| -- | -- | OpenTSDB pro...
[style] ~221-~221: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | |...
[grammar] ~230-~230: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...Forstandalonemode,self_importis recommend to collect metrics generated by itself ...
[style] ~283-~283: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | |...
[grammar] ~292-~292: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...Forstandalonemode,self_importis recommend to collect metrics generated by itself ...
[uncategorized] ~326-~326: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...
[uncategorized] ~326-~326: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | |runtime| -- | -- | The runtime ...
[grammar] ~374-~374: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...e| String |None` | The scope of the google cloud storage.
**It's only used whe...
[grammar] ~375-~375: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...g |None| The credential path of the google cloud storage.
**It's only used whe...
[style] ~415-~415: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | |...
[grammar] ~424-~424: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...Forstandalonemode,self_importis recommend to collect metrics generated by itself ...
Markdownlint
config/config.md
5-5: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
6-6: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
7-7: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
151-151: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
238-238: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
300-300: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
37-37: null (MD034, no-bare-urls)
Bare URL used
185-185: null (MD034, no-bare-urls)
Bare URL used
247-247: null (MD034, no-bare-urls)
Bare URL used
Additional comments not posted (2)
config/config.md (2)
173-173: Clarification Added forgrpc.hostnameThe added comments for the
grpc.hostnamefield enhance understanding by clearly stating its purpose for external connections and service advertisement. This is a positive change for clarity and usability.
318-318: Consistent Clarification Across ConfigurationsSimilar to the previous comment, the clarification added to the
grpc.hostnamein the Datanode section maintains consistency and enhances user understanding of the configuration's purpose.
[APROVED]
|
@zyy17 In our operator, we will need to pass hostname argument to frontend after this patch merged. |
|
@fengjiachun @sunng87 @v0y4g3r Please take another look. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/servers/src/addrs.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/servers/src/addrs.rs
Another heartbeat task, it is for flow engine. @killme2008 |
Fixed |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
#4186
What's changed and what's your intention?
Fixed the wrong peer address that frontend registers.
Checklist
Summary by CodeRabbit
New Features
grpc.hostnameto improve clarity and understanding of its usage for external connections.Improvements
hostnamefield for gRPC servers, enhancing documentation for end-users.Refactor
server_addrtopeer_addrin heartbeat components for consistency.