Skip to content

feat: add --rpc-hostname option to datanode for a persist address to store in meta#871

Merged
fengjiachun merged 3 commits intoGreptimeTeam:developfrom
sunng87:fix/rpc-hostname-for-datanode
Jan 17, 2023
Merged

feat: add --rpc-hostname option to datanode for a persist address to store in meta#871
fengjiachun merged 3 commits intoGreptimeTeam:developfrom
sunng87:fix/rpc-hostname-for-datanode

Conversation

@sunng87
Copy link
Copy Markdown
Member

@sunng87 sunng87 commented Jan 12, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

At the moment, we stored datanode's grpc IP address in meta. However when this address changes, via a deployment in k8s for example, it's impossible for frontend to find that datanode again. This patch adds a new start option --rpc-hostname to datanode startup command and it will use value to report to meta.

  • this option is optional and it fallback to --rpc-addr, this behaviour is completely same to current
  • as its name suggests, you can configure hostname and it resolves port from --rpc-addr
  • user has to guarantee that the --rpc-hostname resolves to --rpc-addr so that frontend can call datanode via this hostname. User can use 0.0.0.0 in --rpc-addr as an exception, if the environment permits

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

@sunng87 sunng87 marked this pull request as ready for review January 12, 2023 11:39
@sunng87 sunng87 marked this pull request as draft January 12, 2023 11:53
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 12, 2023

Codecov Report

Merging #871 (eabdde2) into develop (0e8411c) will decrease coverage by 0.19%.
The diff coverage is 91.66%.

@@             Coverage Diff             @@
##           develop     #871      +/-   ##
===========================================
- Coverage    86.32%   86.13%   -0.20%     
===========================================
  Files          422      425       +3     
  Lines        55669    56672    +1003     
===========================================
+ Hits         48058    48812     +754     
- Misses        7611     7860     +249     
Flag Coverage Δ
rust 86.13% <91.66%> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/datanode/src/instance.rs 49.76% <0.00%> (-16.11%) ⬇️
src/cmd/src/datanode.rs 77.48% <50.00%> (-0.75%) ⬇️
src/datanode/src/datanode.rs 58.92% <100.00%> (+0.74%) ⬆️
src/datanode/src/heartbeat.rs 85.96% <100.00%> (+5.50%) ⬆️
src/datanode/src/mock.rs 100.00% <100.00%> (ø)
src/object-store/src/test_util.rs 0.00% <0.00%> (-100.00%) ⬇️
src/meta-srv/src/handler/collect_stats_handler.rs 81.08% <0.00%> (-13.52%) ⬇️
tests-integration/src/test_util.rs 87.55% <0.00%> (-11.97%) ⬇️
src/datanode/src/sql/insert.rs 80.64% <0.00%> (-9.58%) ⬇️
... and 68 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sunng87
Copy link
Copy Markdown
Member Author

sunng87 commented Jan 12, 2023

The option name may be confusing because it requires port as well. Or we can try to resolve port from --rpc-addr for better usability.

edit: implemeted

@sunng87 sunng87 marked this pull request as ready for review January 12, 2023 13:38
Comment thread src/datanode/src/heartbeat.rs Outdated
Comment thread src/datanode/src/heartbeat.rs Outdated
Co-authored-by: fys <40801205+Fengys123@users.noreply.github.com>
Copy link
Copy Markdown
Collaborator

@fengjiachun fengjiachun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fengjiachun fengjiachun merged commit ecb71f8 into GreptimeTeam:develop Jan 17, 2023
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
…store in meta (GreptimeTeam#871)

* feat: add --rpc-hostname option

* fix: config file and hostname parsing

* Apply suggestions from code review

Co-authored-by: fys <40801205+Fengys123@users.noreply.github.com>

Co-authored-by: fys <40801205+Fengys123@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants