-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Configure internal communication for ANNOUNCE discovery mode #27030
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
base: master
Are you sure you want to change the base?
Conversation
So that it can be reused for other announce modes.
This includes automatic generation of TLS certificates for internal communication when the certificate is not configured explicitly.
Reviewer's GuideIntroduced a reusable module to centralize internal communication setup and enabled automatic TLS certificate generation for ANNOUNCE discovery mode by delegating client configuration and request filters to the new module. Class diagram for InternalCommunicationForDiscoveryModule and related changesclassDiagram
class InternalCommunicationForDiscoveryModule {
-Class<? extends Annotation> httpClientQualifier
+InternalCommunicationForDiscoveryModule(httpClientQualifier)
+setup(Binder binder)
}
class DiscoveryEncodeAddressAsHostname {
+filterRequest(Request request) Request
-toIpEncodedAsHostnameUri(URI uri) URI
}
InternalCommunicationForDiscoveryModule <|-- DiscoveryEncodeAddressAsHostname
class InternalCommunicationConfig {
+isHttpsRequired() boolean
+getKeyStorePath() String
+getTrustStorePath() String
}
class InternalAuthenticationManager
class HttpClientConfig
class HttpRequestFilter
InternalCommunicationForDiscoveryModule --> InternalCommunicationConfig
InternalCommunicationForDiscoveryModule --> HttpClientConfig
InternalCommunicationForDiscoveryModule --> HttpRequestFilter
InternalCommunicationForDiscoveryModule --> InternalAuthenticationManager
DiscoveryEncodeAddressAsHostname ..|> HttpRequestFilter
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Consider renaming InternalCommunicationForDiscoveryModule (and its private DiscoveryEncodeAddressAsHostname) to more descriptive, reusable names (e.g. InternalCommunicationHttpClientModule / IpToHostnameRequestFilter) to clarify intent and enable future reuse.
- Extract DiscoveryEncodeAddressAsHostname into a top-level, testable filter class so that DNS-to-hostname translation logic can be reused and validated in isolation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider renaming InternalCommunicationForDiscoveryModule (and its private DiscoveryEncodeAddressAsHostname) to more descriptive, reusable names (e.g. InternalCommunicationHttpClientModule / IpToHostnameRequestFilter) to clarify intent and enable future reuse.
- Extract DiscoveryEncodeAddressAsHostname into a top-level, testable filter class so that DNS-to-hostname translation logic can be reused and validated in isolation.
## Individual Comments
### Comment 1
<location> `core/trino-main/src/main/java/io/trino/node/InternalCommunicationForDiscoveryModule.java:71` </location>
<code_context>
- .build();
- }
-
- private static URI toIpEncodedAsHostnameUri(URI uri)
- {
- if (!uri.getScheme().equals("https")) {
</code_context>
<issue_to_address>
**issue:** Potential edge case if uri.getHost() returns null.
Check for a null host before calling InetAddress.getByName to prevent exceptions.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Description
This includes automatic generation of TLS certificates for internal communication when the certificate is not configured explicitly in the
ANNOUNCE
node discovery mode.Additional context and related issues
This follows up on the node inventory refactoring in #26083.
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:
Summary by Sourcery
Enable automatic TLS setup for internal communications in both node discovery and announce modes by extracting and applying the configuration logic into a reusable module.
New Features:
Enhancements: