Skip to content
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

Add support for config provider for any property #1039

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VitoMakarevich
Copy link

Overview

SNOW-XXXXX

Currently, config providers are allowed for connection-related properties only - for any other property, validation runs and fails, as it's run before the config provider resolves values.
This PR adds a check of validation error and if it's caused by configuration provider - removes the error.
Unfortunately - the way it's done is hacky(uses some bad encapsulation from KafkaConnect authors).
I can find a better way to do so(such as rewriting code to extend each validator from a config-aware one, so that the value is checked only if it's not supposed to be resolved by the config provider).
Please let me know if this improvement can be accepted on your side, and I'll adjust the code as you'd like.

The motivation is that likely user should be able to provide any config parameter using the config provider for whatever reason, and there should be no hidden assumptions about the secrecy of the parameters.
P.S. The true motivation in my case is that we are planning to consume from ~100 topics in one setup, and the connector's config is just too long for AWS MSK to handle(not sure whether this is MSK or KafkaConnect itself). The issue is exaggerated by an even bigger topic2table map(compared to a list of topics).
P.P.S. I was also considering why validation happens before config resolution, but cannot find meaningful results. Also I was considering to resolve these values by writing some code(it's possible 100%) - but didn't find existing KafkaConnect Connectors to do so(checked JDBC/S3 sinks).

Pre-review checklist

  • This change should be part of a Behavior Change Release. See go/behavior-change.
  • This change has passed Merge gate tests
  • Snowpipe Changes
  • Snowpipe Streaming Changes
  • This change is TEST-ONLY
  • This change is README/Javadocs only
  • This change is protected by a config parameter <PARAMETER_NAME> eg snowflake.ingestion.method.
    • Yes - Added end to end and Unit Tests.
    • No - Suggest why it is not param protected
  • Is his change protected by parameter <PARAMETER_NAME> on the server side?
    • The parameter/feature is not yet active in production (partial rollout or PrPr, see Changes for Unreleased Features and Fixes).
    • If there is an issue, it can be safely mitigated by turning the parameter off. This is also verified by a test (See go/ppp).

Likely no risks, as existing code will remain the same, while for some portion of users it may be useful.

@VitoMakarevich VitoMakarevich requested a review from a team as a code owner January 7, 2025 19:34
@VitoMakarevich
Copy link
Author

Hi @sfc-gh-mbobowski @sfc-gh-achyzy could you please review this

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.

1 participant