-
Notifications
You must be signed in to change notification settings - Fork 11
feat(huntsman)!: Replace endpoint's IP address with general hostname. #401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
eaadd32
8e4456f
27b7e7a
233e43b
9d695cb
016bf89
c8d01f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ use spider_core::types::id::ResourceGroupId; | |
| use spider_core::types::id::SessionId; | ||
| use spider_core::types::id::TaskId; | ||
| use spider_core::types::id::TaskInstanceId; | ||
| use spider_utils::config::Host; | ||
| use tonic::Code; | ||
|
|
||
| use crate::storage::AddResourceGroupRequest; | ||
|
|
@@ -185,21 +186,16 @@ impl RequestUnpack for ExecutionManagerIdRequest { | |
| } | ||
| } | ||
|
|
||
| /// Unpacks [`RegisterSchedulerRequest`] into a tuple containing: | ||
| /// | ||
| /// * The scheduler IP address. | ||
| /// * The scheduler port. | ||
| /// Unpacks [`RegisterSchedulerRequest`] into the scheduler host and port. | ||
| impl RequestUnpack for RegisterSchedulerRequest { | ||
| type Unpacked = (IpAddr, u16); | ||
| type Unpacked = (Host, u16); | ||
|
|
||
| fn unpack(self) -> Result<Self::Unpacked, UnpackError> { | ||
| let ip_address = self | ||
| .ip_address | ||
| .parse::<IpAddr>() | ||
| .map_err(|error| invalid_argument(format!("invalid IP address: {error}")))?; | ||
| let host = Host::new(self.host) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if DNS failed to resolve the URL. This failure is often seen in the networking, and we should be able to deal with the gracefully
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the DNS name is syntactically wrong, then in If the DNS name is syntactically correct, and not an existing address that is connectable, then the gRPC client's I am not sure if these are graceful enough in your opinion.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its good that you are thinking about this, but maybe its my problem that I dont really understand what exactly is this "error" you are talking about. can you tell me say if I input an invalid ip or user name, what would the user see? I am okay if you think what you presented to user can clearly show what is the root cause.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For example, if user set an invalid storage hostname in scheduler config, scheduler will fail with "Failed to parse storage endpoint." or "Failed to connect to storage gRPC service.". It also includes the message from underlying library. |
||
| .map_err(|_| invalid_argument("host must not be empty".to_owned()))?; | ||
| let port = u16::try_from(self.port) | ||
| .map_err(|_| invalid_argument(format!("port does not fit in `u16`: {}", self.port)))?; | ||
| Ok((ip_address, port)) | ||
| Ok((host, port)) | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what if the input is an IP, and the IP is invalid, would deleting the check cause any regression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no good way to tell if it is an invalid IP or just not an IP address. I think the best way is to leave it to the other functions mentioned in #401 (comment) to handle the failure.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really? isnt that you are doing this in the deleted line?
Say user inputs 256.256.256.256, this is not a valid IP right? We can easily parse it (of course though some lib) and let the user knows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, it can only be an IPv4, Thus, we know for sure if it is not a valid IPv4 address, it is invalid. The
AddrParseErroris not specific enough to inform us if it is a invalid IP address out of range, or just something totally does not look like an IP address.