Add support for Spanner Omni#153
Conversation
|
Welcome as a new contributor to Debezium, @sagnghos. Reviewers, please add missing author name(s) and alias name(s) to the COPYRIGHT.txt and Aliases.txt respectively. |
|
Hi @sagnghos, thanks for your contribution. Please prefix the commit message(s) with the debezium/dbz#xxx GitHub issue key. |
sagnghos
force-pushed
the
sagnghos/spannerOmni
branch
from
11b6a9a to
ad5cba0
Compare
Signed-off-by: sagnghos <sagnikghosh@google.com>
sagnghos
force-pushed
the
sagnghos/spannerOmni
branch
from
ad5cba0 to
e8f5824
Compare
| .withImportance(Importance.LOW) | ||
| .withDescription("Path to the client key file for Spanner Omni connection"); | ||
|
|
||
| public static final Field SPANNER_OMNI_CLIENT_CERT_PATH = Field.create(SPANNER_OMNI_CLIENT_CERT_PATH_PROPERTY_NAME) |
There was a problem hiding this comment.
There should be a custom validator that will ensure (IIRC the following code correctly)
- plaintext is false and both key and cert paths are present
- plaintext is true and neither key nor cert are present
There was a problem hiding this comment.
I have added a SpannerOmniValidator class
Scenarios validated:
- Clear any errors related to instance and project when spanner omni endpoint is set, as project and instance is always "default". Reference: https://docs.cloud.google.com/spanner-omni/client-library-overview
- If spanner omni endpoint is NOT set NONE of (plaintext, client cert and key) should be set
- When plain text is true, key and cert shouldn't be set
- key and cert should always be provided together
The scenario where Spanner Omni endpoint is set but use plain text is false and client cert and key are not provided is a valid scenario as it is the scenario for TLS communcation
In summary for Spanner Omni - The Spanner Omni endpoint should always be set
- Plain Text : Set use plain text to true
- TLS: No addtional parameters need to be set other than the Spanner Omni endpoint
- mTLS: Client Key and Cert needs to be provided
Signed-off-by: sagnghos <sagnikghosh@google.com>
Signed-off-by: sagnghos <sagnikghosh@google.com>
| .with("heartbeat.interval.ms", "300000") | ||
| .with("gcp.spanner.low-watermark.enabled", false) | ||
| .with("tasks.max", 3) // see DBZ-8428 | ||
| .with("spanner.omni.endpoint", Database.getSpannerOmniEndpoint()) |
There was a problem hiding this comment.
Is this change needed? Because getSpannerOmniEndpoint() returns null here (be default), and currently we don't have any IT tests using spanner omni.
| builder.useClientCert(clientCertPath, clientKeyPath); | ||
| } | ||
| } | ||
| else if (!Strings.isNullOrEmpty(emulatorHost)) { |
There was a problem hiding this comment.
Spanner Omni endpoint will override emulator endpoint. Probably we should add some comments for this? Thanks!
| } | ||
|
|
||
| if (!Strings.isNullOrEmpty(databaseRole)) { | ||
| if (!Strings.isNullOrEmpty(databaseRole) && Strings.isNullOrEmpty(spannerOmniEndpoint)) { |
There was a problem hiding this comment.
Maybe add some comment that databaseRole is not supported by Spanner Omni? Are you going to support it in the future? Thanks!
|
|
||
| private String createInstance() { | ||
| if (isSpannerOmniEndpoint()) { | ||
| return DatabaseClientFactory.SPANNER_OMNI_DEFAULT_ID; |
There was a problem hiding this comment.
I'm curious - whenever we are using Omni, the project / instance names will always be the default values, not user-specified ones?
| void testConfig() { | ||
| ConfigDef actualConfigResult = new SpannerConnector().config(); | ||
| Map<String, ConfigDef.ConfigKey> configKeysResult = actualConfigResult.configKeys(); | ||
| assertEquals(57, configKeysResult.size()); |
There was a problem hiding this comment.
Two questions:
- Can we have a TODO to add an integration test for Omni? (See tests that end with IT.java).
- Will we eventually refactor all integration tests in this repo to use Omni vs. the emulator?
| /** | ||
| * Validates Spanner Omni configuration properties to ensure they follow the correct usage patterns. | ||
| */ | ||
| public class SpannerOmniValidator implements ConfigurationValidator.Validator { |
There was a problem hiding this comment.
I'm not quite sure I like the concept of clearing errors in the Omni validator.
Would it make sense to have two validators: SpannerOmniValidator, and CloudSpannerValidator? We can then branch between the two validators based on the existence of Omni-specific parameters within ConfigurationValidator?
4 participants
Make debezium-connector-spanner compatible with Spanner Omni
Fixes debezium/dbz#1858