Copilot commented on code in PR #17703:
URL: https://github.com/apache/pinot/pull/17703#discussion_r2824271660
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/SegmentPushUtils.java:
##########
@@ -79,6 +85,35 @@ private SegmentPushUtils() {
private static final Logger LOGGER =
LoggerFactory.getLogger(SegmentPushUtils.class);
private static final FileUploadDownloadClient FILE_UPLOAD_DOWNLOAD_CLIENT =
new FileUploadDownloadClient();
+ static FileUploadDownloadClient getFileUploadDownloadClient(TlsSpec tlsSpec)
{
+ if (tlsSpec == null) {
+ return FILE_UPLOAD_DOWNLOAD_CLIENT;
+ }
+ return new FileUploadDownloadClient(buildSSLContext(tlsSpec));
+ }
Review Comment:
`getFileUploadDownloadClient()` creates a new `FileUploadDownloadClient`
(and underlying pooled `HttpClient`) when `tlsSpec != null`, but none of the
call sites close it. Because `FileUploadDownloadClient` implements
`AutoCloseable`, this will leak connection pools/threads in long-running JVMs.
Consider creating the client in a try-with-resources (only when `tlsSpec !=
null`) and closing it after the push/send completes, or caching a
per-job/per-spec client and closing it in a finally block. Also consider
returning the shared `FILE_UPLOAD_DOWNLOAD_CLIENT` when `tlsSpec` is non-null
but does not configure a keystore/truststore to avoid unnecessary client
creation.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/ConsistentDataPushUtils.java:
##########
@@ -108,6 +107,8 @@ public static Map<URI, URI>
getStartReplaceSegmentUris(SegmentGenerationJobSpec
public static Map<URI, String> startReplaceSegments(SegmentGenerationJobSpec
spec,
Map<URI, List<String>> uriToSegmentsFrom, List<String> segmentsTo)
throws Exception {
+ FileUploadDownloadClient fileUploadDownloadClient =
+ SegmentPushUtils.getFileUploadDownloadClient(spec.getTlsSpec());
Review Comment:
This method now obtains a `FileUploadDownloadClient` potentially backed by a
new pooled `HttpClient` (when `spec.getTlsSpec() != null`), but it is never
closed. Please ensure the client is closed after the retry loop completes
(e.g., try-with-resources or a finally block), otherwise repeated
consistent-push operations can leak HTTP connection managers/threads.
##########
pinot-common/src/main/java/org/apache/pinot/common/segment/generation/SegmentGenerationUtils.java:
##########
@@ -257,13 +258,19 @@ public static String fetchUrl(URL url, String authToken,
TlsSpec tlsSpec)
HttpsURLConnection httpsConn = (HttpsURLConnection) connection;
if (tlsSpec != null) {
+ KeyManager[] keyManagers = null;
+ if (tlsSpec.getKeyStorePath() != null) {
+ keyManagers = TlsUtils.createKeyManagerFactory(
+ tlsSpec.getKeyStorePath(), tlsSpec.getKeyStorePassword(),
tlsSpec.getKeyStoreType())
+ .getKeyManagers();
+ }
TrustManagerFactory tmf = TlsUtils.createTrustManagerFactory(
tlsSpec.getTrustStorePath(),
tlsSpec.getTrustStorePassword(),
tlsSpec.getTrustStoreType());
Review Comment:
`fetchUrl()` unconditionally builds a `TrustManagerFactory` from
`tlsSpec.getTrustStorePath()` when `tlsSpec != null`.
`TlsUtils.createTrustManagerFactory(...)` requires non-null truststore
path/password and will throw if a job only configures a keystore (client cert)
and relies on default JVM trust. Please make truststore optional here (similar
to `SegmentPushUtils.buildSSLContext`) by only creating the TMF when a
truststore path is provided, otherwise fall back to default trust managers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
