-
Couldn't load subscription status.
- Fork 14.7k
KAFKA-19824: New AllowlistConnectorClientConfigOverridePolicy (KIP-1188) #20750
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: trunk
Are you sure you want to change the base?
Conversation
|
cc @C0urante @AndrewJSchofield @showuon as you voted on the KIP. Thanks |
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.
Thanks Mickael, only comment is about making the upgrade path clearer for users currently on the principal policy. Everything else looks good to me
| * {@code Principal}. This allows to set a principal per connector. | ||
| * @deprecated Use {@link AllowlistConnectorClientConfigOverridePolicy} instead. | ||
| */ | ||
| @Deprecated(since = " 4.2", forRemoval = true) |
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.
Can we add a warning log message when this class is instantiated to notify users about its deprecation and what steps they should take to prepare for its removal?
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.
Good idea, done
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.
Thanks for the PR. A couple of nits only. Looks good to me.
| public static final String CONNECTOR_CLIENT_POLICY_CLASS_CONFIG = "connector.client.config.override.policy"; | ||
| public static final String CONNECTOR_CLIENT_POLICY_CLASS_DOC = | ||
| "Class name or alias of implementation of <code>ConnectorClientConfigOverridePolicy</code>. Defines what client configurations can be " | ||
| + "overridden by the connector. The default implementation is <code>All</code>, meaning connector configurations can override all client properties. " |
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.
nit: "The default policy" reads a little better than "The default implementation" given that the others are described as policies.
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.
I agree, updated. Thanks
| ConfigInfos result = herder.validateConnectorConfig(config, s -> null, false); | ||
| assertEquals(ConnectorType.SOURCE, herder.connectorType(config)); | ||
|
|
||
| // We expect there to be errors due to sasl.jaas.config not being allowed Note that these assertions depend heavily on |
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.
nit: Missing "." before "Note"
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.
Fixed
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.
LGTM!
Implements KIP-1188