Skip to content

[WIP] CC-33579 - use connector level proxy configs and make HttpClient keyed on proxy #35

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

Draft
wants to merge 1 commit into
base: v2.1.0-hotfix-x
Choose a base branch
from

Conversation

sangeet259
Copy link
Member

@sangeet259 sangeet259 commented May 28, 2025

To be added
Ignore the log lines. They are currently for debugging purposes. Will remove them before the final state.

@sangeet259 sangeet259 changed the title wip commit wip May 28, 2025
@sangeet259 sangeet259 changed the title wip [WIP] use connector level proxy configs and make HttpClient keyed on proxy May 28, 2025
@sangeet259 sangeet259 changed the title [WIP] use connector level proxy configs and make HttpClient keyed on proxy [WIP] CC-33579 - use connector level proxy configs and make HttpClient keyed on proxy May 28, 2025
@sangeet259 sangeet259 requested a review from Copilot May 29, 2025 03:30
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements connector-level proxy configuration support and updates the HTTP client caching mechanism to key clients by proxy settings.

  • Introduces HttpClientSettingsKey and a concurrent cache in HttpUtil to manage multiple CloseableHttpClient instances based on account and proxy settings.
  • Updates Utils.createProperties to preserve case for proxy-related properties and propagates proxyProperties through new constructors in ingestion and streaming classes.
  • Adds comprehensive tests in HttpUtilCacheTest to verify HTTP client caching behavior with different proxy scenarios.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/main/java/net/snowflake/ingest/utils/HttpUtil.java Use proxy-aware HttpClient cache keyed by HttpClientSettingsKey.
src/main/java/net/snowflake/ingest/utils/Utils.java Preserve case for proxy-related properties in property normalization.
src/main/java/net/snowflake/ingest/utils/HttpClientSettingsKey.java New serializable key class for HTTP client settings.
src/main/java/net/snowflake/ingest/streaming/internal/StreamingIngestStage.java Add constructor overload and propagate proxyProperties.
src/main/java/net/snowflake/ingest/streaming/internal/SnowflakeStreamingIngestClientInternal.java Store and extract original proxy properties for client creation.
src/main/java/net/snowflake/ingest/streaming/internal/FlushService.java Pass proxyProperties into StreamingIngestStage.
src/main/java/net/snowflake/ingest/SimpleIngestManager.java Add constructor and init method support for proxyProperties.
src/test/java/net/snowflake/ingest/utils/HttpUtilCacheTest.java New tests for HTTP client caching with and without proxy settings.
pom.xml Update groupId to net.snowflake and version to local snapshot.
Comments suppressed due to low confidence (6)

src/test/java/net/snowflake/ingest/utils/HttpUtilCacheTest.java:20

  • Consider clearing system proxy-related properties in tearDown to avoid test flakiness if system properties affect HttpUtil behavior.
@After

src/main/java/net/snowflake/ingest/utils/HttpUtil.java:117

  • Remove or replace these System.out.println debug statements with proper logger calls and avoid shipping debug-only prints.
    System.out.println("DEBUGSNOWFLAKEPROXY: From Snowflake Ingest HttpUtil. Getting HTTP client for account: " + accountName);

src/main/java/net/snowflake/ingest/utils/HttpUtil.java:176

  • Avoid logging sensitive information such as proxy passwords; these debug prints expose credentials in plain text.
        System.out.println("DEBUGSNOWFLAKEPROXY: From Snowflake Ingest HttpUtil. Proxy Password: " + proxyPassword);

src/main/java/net/snowflake/ingest/utils/Utils.java:144

  • [nitpick] Inconsistent indentation in the new isProxyRelatedProperty branch; align braces and statements with surrounding code style conventions.
            properties.put(key, val);

src/main/java/net/snowflake/ingest/streaming/internal/StreamingIngestStage.java:131

  • Multiple System.out.println debug statements should be removed or converted to logger.debug to follow logging best practices.
    System.out.println("DEBUGSNOWFLAKEPROXY: StreamingIngestStage constructor - final proxyProperties contains " + this.proxyProperties.size() + " properties:");

src/main/java/net/snowflake/ingest/streaming/internal/SnowflakeStreamingIngestClientInternal.java:178

  • Remove or replace these System.out.println debug statements with appropriate logging; avoid excessive debug output in production code.
    System.out.println("DEBUGSNOWFLAKEPROXY: Properties are not null in SnowflakeStreamingIngestClientInternal");

", proxyPort=" + proxyPort +
", nonProxyHosts='" + nonProxyHosts + '\'' +
", proxyUser='" + proxyUser + '\'' +
", proxyPassword=" + (proxyPassword.isEmpty() ? "not set" : "set") +
Copy link
Member

Choose a reason for hiding this comment

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

nit -> lets no print password

Copy link
Member Author

Choose a reason for hiding this comment

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

sure. this is all for debugging. will remove.

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.

2 participants