feat(Spanner): integrate SourceConfigParser to centralize shard configuration loading for SourceDbToSpanner pipelines. - #3854
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3854 +/- ##
============================================
- Coverage 56.10% 56.09% -0.01%
+ Complexity 7319 7310 -9
============================================
Files 1126 1126
Lines 68550 68555 +5
Branches 7753 7754 +1
============================================
+ Hits 38457 38459 +2
+ Misses 27610 27609 -1
- Partials 2483 2487 +4
🚀 New features to boost your workflow:
|
453a9f9 to
b7b20a3
Compare
a19cf9e to
581ffd3
Compare
0da7475 to
e13bdd2
Compare
c90217f to
c58f5cb
Compare
41f2355 to
461938b
Compare
c7f9560 to
2649e0a
Compare
| * @throws IllegalArgumentException if the provided options are invalid for the pipeline. | ||
| */ | ||
| @VisibleForTesting | ||
| static void validateOptions( |
There was a problem hiding this comment.
couple of points.
- This looks very very similar to the previous method, why is this created as a different method here ? It would be better to just tweak the existing method for this extra parameter to keep the amount of change less ?
- As this is PG specific code, it should ideally go into the source directory and be exposed via the src connector. Please check if that is feasible here.
There was a problem hiding this comment.
Have added the TO-DO and moved the method.
| () -> | ||
| CassandraIOWrapperHelper.buildDataSource( | ||
| TEST_BUCKET_CASSANDRA_CONFIG_CONF, | ||
| null, |
There was a problem hiding this comment.
It doesn't seem like there is a test with this TEST_BUCKET_CASSANDRA_CONFIG_CONF config ? Can you double check ?
| .oss() | ||
| .numPartitions()) | ||
| .isEqualTo(42); | ||
| assertThrows( |
There was a problem hiding this comment.
Is there an alternate test for invalid config url ?
| ResourceManagerUtils.cleanResources(spannerResourceManager, mySQLResourceManager); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
is this comment not relevant anymore ?
There was a problem hiding this comment.
Yes, SHARD Id is populated. See expected data at line 106
| shard.setLogicalShardId("Shard1"); | ||
| shard.setUser(jdbcResourceManager.getUsername()); | ||
| shard.setPassword(jdbcResourceManager.getPassword()); | ||
| if (jdbcResourceManager instanceof PostgresResourceManager pgRm) { |
There was a problem hiding this comment.
please check if the test cleanup scripts will handle these directories
There was a problem hiding this comment.
We are not creating any new folder here. This uses the run-id folder used by all other files.
protected String getGcsPath(String artifactId) {
return ArtifactUtils.getFullGcsPath(
artifactBucketName, getClass().getSimpleName(), gcsClient.runId(), artifactId);
}
|
|
||
| JdbcShardConfig jdbcConfig = new JdbcShardConfig(); | ||
| Shard shard1 = new Shard(); | ||
| shard1.setNamespace("public"); |
There was a problem hiding this comment.
do we have a test for non-public namespaces as well ?
There was a problem hiding this comment.
This current test is testing non-public name space (line:155),
At line:166 only public name space is getting tested.
…guration loading for SourceDbToSpanner pipelines
…nectionConfig instead of pipeline options for improved configuration handling.
…es and update JDBC migration logic to utilize shard configurations
…object directly to builder methods
2649e0a to
157262e
Compare
f2d0565 to
2ddbd87
Compare
…e standardized shard config GCS file
23b9871 to
7007b5f
Compare
Uh oh!
There was an error while loading. Please reload this page.