CCDB-4354: preflight cross-fields validation for configuration PK_MODE #1157
Conversation
There was a problem hiding this comment.
Thanks @fzhong-connect. I've left a few comments; anything beginning with "nit: " can be implemented or ignored at your discretion. The only significant one has to do with providing clear error messages to users; let me know what you think.
Also, what's the reasoning for targeting 10.2.x with this change? Based on https://docs.confluent.io/platform/current/installation/versions-interoperability.html#cp-and-apache-ak-compatibility, we're still providing support as far back as 5.2.x; if it's not too difficult with merge conflicts and the like, could we consider targeting that branch instead?
| .findFirst() | ||
| .orElse(Collections.emptyList()); | ||
|
|
||
| assertTrue(pkModeError.isEmpty()); |
There was a problem hiding this comment.
Nit: I prefer to do a comparison for tests like this since it gives a more informative message if the assertion fails:
| assertTrue(pkModeError.isEmpty()); | |
| assertEquals(Optional.empty(), pkModeError); |
| .findFirst() | ||
| .orElse(Collections.emptyList()); | ||
|
|
||
| assertTrue(pkModeError.isEmpty()); |
There was a problem hiding this comment.
(Same nit RE: comparison for assertion)
| assertTrue(pkModeError.isEmpty()); | |
| assertEquals(Optional.empty(), pkModeError); |
src/test/java/io/confluent/connect/jdbc/JdbcSinkConnectorTest.java
Outdated
Show resolved
Hide resolved
Thank you for taking time @C0urante . The reason it's targeting 10.2.x is Nathan believes it mostly benefits user of the fully-managed connector on CCloud, he asked to start from the latest version on CC. |
| if (!pkMode.recommendedValues().contains(pkMode.value())) { | ||
| pkMode.addErrorMessage("'" + pkMode.value() + "' is invalid:" | ||
| + " Deletes are only supported for pk.mode 'record_key'"); | ||
| } |
There was a problem hiding this comment.
Is this susceptible to false positives if the user gives a value for the pk.mode property that is invalid not because of conflict with delete.enabled but just because it doesn't correspond to a recognized primary key mode? For example, something like pk.mode = not_a_real_mode.
There was a problem hiding this comment.
that is great catch! thank you.
Problem
Misconfigured
pk.modeanddelete.enabledcombination should not be surfaced by throwing an exception when the connector or task starts. Instead, the connector should notify the user that it is configured incorrectly via the Connector::validate method.Solution
Do cross-fields configuration validation in
Connector::validatemethod, utilizing existing logic from PrimaryKeyModeRecommender.Ref:
Does this solution apply anywhere else?
If yes, where?
Test Strategy
Testing done:
Release Plan