Ability to set the schema pattern for tables metadata retrieval #109

pqueixalos · 2016-08-11T11:50:42Z

This adds the ability to configure jdbc schemaPattern from connector configuration.
Reported here #32

…ble metadata

ConfluentJenkins · 2016-08-11T11:50:43Z

Can one of the admins verify this patch?

ghost · 2016-08-11T11:50:44Z

It looks like @pqueixalos hasn't signed our Contributor License Agreement, yet.

Appreciation of efforts,

clabot

pqueixalos · 2016-08-11T18:31:07Z

CLA just signed.

shikhar · 2016-08-19T00:11:10Z

src/main/java/io/confluent/connect/jdbc/source/JdbcSourceConnectorConfig.java

@@ -144,6 +143,9 @@
  public static final long TIMESTAMP_DELAY_INTERVAL_MS_DEFAULT = 0;
  private static final String TIMESTAMP_DELAY_INTERVAL_MS_DISPLAY = "Delay Interval (ms)";

+  public static final String SCHEMA_PATTERN_CONFIG = "schemaPattern";


schema.pattern to be consistent with the config naming conventions

shikhar · 2016-08-19T00:15:15Z

@pqueixalos is it possible to just include the schema in the connection URL?

pqueixalos · 2016-08-19T08:18:26Z

@shikhar, short answer : no.
Settting the schema in URL configures the JDBC Driver and not all JDBC implementations supports this.
Also it would not change anything (I tried :)), the main issue is that JdbcUtils is aksing for database metadata without providing any schema pattern so every tables of every 'usable' schemas are retrieved.

Regarding the changes you asked (whitespace and parameter naming, sure, I will make those changes next week).

shikhar · 2016-08-19T13:24:26Z

@pqueixalos would it work to update the code to use connection.getSchema() as the schema pattern? (in conjunction with the user specifying it as part of the connection URL)

shikhar · 2016-08-19T13:25:41Z

src/main/java/io/confluent/connect/jdbc/JdbcSourceConnector.java

@@ -16,6 +16,12 @@

 package io.confluent.connect.jdbc;

+import io.confluent.connect.jdbc.source.JdbcSourceConnectorConfig;


let's avoid the import rearrangement

pqueixalos · 2016-08-22T08:13:56Z

Good point @shikhar, getting the schema from the connection (when set in jdbc connection params) might work fine in some situations !
It won't work in my context for sure, as (silly) Redshift connector does not support it. Also, I don't know how much other jdbc connector implementations honor this, for instance MySQL would probably return null as it does not implement schema concept, connection.getCatalog() would need to be used instead.

…ention

ghost · 2016-08-22T16:48:13Z

@confluentinc It looks like @pqueixalos just signed our Contributor License Agreement. 👍

Always at your service,

clabot

shikhar · 2016-08-22T18:14:14Z

src/main/java/io/confluent/connect/jdbc/JdbcSourceConnector.java

@@ -104,8 +105,8 @@ public void start(Map<String, String> properties) throws ConnectException {
      // query.
      whitelistSet = Collections.emptySet();
    }
-    tableMonitorThread = new TableMonitorThread(db, context, tablePollMs, whitelistSet,
-                                                blacklistSet, tableTypesSet);
+    tableMonitorThread = new TableMonitorThread(db, schemaPattern, context, tablePollMs,


@pqueixalos sorry didn't notice this minor thing previously, but in terms of arg ordering schemaPattern makes sense after the context

shikhar · 2016-08-22T18:20:39Z

@pqueixalos ah too bad Redshift JDBC does not support that. So we need this patch to make the JDBC source connector usable against Redshift then.

Left a couple more comments, otherwise LGTM

cc @ewencp in case you have any thoughts on this

shikhar · 2016-08-30T20:00:03Z

ok to test

ewencp · 2016-08-31T01:13:54Z

src/main/java/io/confluent/connect/jdbc/source/JdbcSourceConnectorConfig.java

@@ -190,6 +192,7 @@ public static ConfigDef baseConfigDef() {
        .define(BATCH_MAX_ROWS_CONFIG, Type.INT, BATCH_MAX_ROWS_DEFAULT, Importance.LOW, BATCH_MAX_ROWS_DOC, CONNECTOR_GROUP, 2, Width.SHORT, BATCH_MAX_ROWS_DISPLAY)
        .define(TABLE_POLL_INTERVAL_MS_CONFIG, Type.LONG, TABLE_POLL_INTERVAL_MS_DEFAULT, Importance.LOW, TABLE_POLL_INTERVAL_MS_DOC, CONNECTOR_GROUP, 3, Width.SHORT, TABLE_POLL_INTERVAL_MS_DISPLAY)
        .define(TOPIC_PREFIX_CONFIG, Type.STRING, Importance.HIGH, TOPIC_PREFIX_DOC, CONNECTOR_GROUP, 4, Width.MEDIUM, TOPIC_PREFIX_DISPLAY)
+        .define(SCHEMA_PATTERN_CONFIG, Type.STRING, null, Importance.MEDIUM, SCHEMA_PATTERN_DOC, DATABASE_GROUP, 5, Width.SHORT, QUERY_DISPLAY)


Is this in the right location? If it is part of the DATABASE_GROUP should we relocate it with that group? And I think the order might not be quite right since it looks like there are only 3 items in the DATABASE_GROUP so far. (It'd also be nice to include a DISPLAY setting as well for a more human-friendly form.)

ewencp · 2016-08-31T01:14:57Z

@pqueixalos @shikhar Only took a quick scan through, but the changes seem fine. Major missing piece seems to be tests -- there were some updates to tests but it looks like they just use null for the schema pattern so we haven't actually tested the case where the user sets the schema pattern.

…ntation

…ookup

pqueixalos · 2016-08-31T10:00:24Z

@ewencp I added a test in JdbcUtilsTest, but taht is probably not enough. Making sure that the configuration is well handled at JdbcSourceConnector level would be better. I will take care of this on a later commit.
I also improved the configuration documentation as you suggested, hoping I did well.

…validation

shikhar · 2016-09-06T18:25:44Z

src/main/java/io/confluent/connect/jdbc/source/JdbcSourceConnectorConfig.java

+      "Schema pattern to fetch tables metadata from:\n"
+      + "  * \"\" retrieves those without a schema,"
+      + "  * null (default) means that the schema name should not be used to narrow the search, all tables "
+      + "metadata would be fetched, regardless there schema.";


typo: s/there/their

shikhar · 2016-09-06T18:31:38Z

@pqueixalos a few minor comments about the doc, otherwise LGTM

shikhar · 2016-09-07T17:26:21Z

Thanks for the contrib @pqueixalos!

jmfn

👍

Added schemaPattern connector task configuration, used for fetched ta…

fef6e74

…ble metadata

shikhar reviewed Aug 19, 2016
View reviewed changes

Pablo Queixalos added 2 commits August 22, 2016 18:20

changed schema pattern configuration property id to honor naming conv…

64847d5

…ention

reverted import rearrangement and identation changes

0455f38

pqueixalos changed the title ~~Added schemaPattern connector task configuration, used for fetched ta…~~ Ability to set the schema pattern for tables metadata retrieval Aug 22, 2016

shikhar reviewed Aug 22, 2016
View reviewed changes

Pablo Queixalos added 2 commits August 23, 2016 09:42

improved TableMonitorThread constructor signature

440e9df

remove useless attribute member in JdbcSourceTask.

23680b3

ewencp reviewed Aug 31, 2016
View reviewed changes

Pablo Queixalos added 2 commits August 31, 2016 10:57

Fixed ordering and improved schema.pattern configuration entry docume…

04b193c

…ntation

added missing test regarding schema pattern narrowed table metadata l…

1e01861

…ookup

added test that asserts the schema pattern is used for configuration …

9e438ac

…validation

shikhar reviewed Sep 6, 2016
View reviewed changes

fixed and improved schema pattern configuration property documentation

54ada71

shikhar merged commit 7f0e59d into confluentinc:master Sep 7, 2016

shikhar mentioned this pull request Sep 13, 2016

Schema name prefixes - fixes #123 #124

Closed

jmfn reviewed Sep 17, 2016

View reviewed changes

shikhar mentioned this pull request Oct 26, 2016

Database schema support #32

Closed

gharris1727 mentioned this pull request Dec 16, 2019

Fix Postgres OOM by limiting fetch size #758

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to set the schema pattern for tables metadata retrieval #109

Ability to set the schema pattern for tables metadata retrieval #109

pqueixalos commented Aug 11, 2016

ConfluentJenkins commented Aug 11, 2016

ghost commented Aug 11, 2016

pqueixalos commented Aug 11, 2016

shikhar Aug 19, 2016

shikhar commented Aug 19, 2016

pqueixalos commented Aug 19, 2016

shikhar commented Aug 19, 2016

shikhar Aug 19, 2016

pqueixalos commented Aug 22, 2016

ghost commented Aug 22, 2016

shikhar Aug 22, 2016

shikhar commented Aug 22, 2016

shikhar commented Aug 30, 2016

ewencp Aug 31, 2016

ewencp commented Aug 31, 2016

pqueixalos commented Aug 31, 2016

shikhar Sep 6, 2016

shikhar commented Sep 6, 2016

shikhar commented Sep 7, 2016

jmfn left a comment

		@@ -16,6 +16,12 @@

		package io.confluent.connect.jdbc;

		import io.confluent.connect.jdbc.source.JdbcSourceConnectorConfig;

Ability to set the schema pattern for tables metadata retrieval #109

Ability to set the schema pattern for tables metadata retrieval #109

Conversation

pqueixalos commented Aug 11, 2016

ConfluentJenkins commented Aug 11, 2016

ghost commented Aug 11, 2016

pqueixalos commented Aug 11, 2016

shikhar Aug 19, 2016

Choose a reason for hiding this comment

shikhar commented Aug 19, 2016

pqueixalos commented Aug 19, 2016

shikhar commented Aug 19, 2016

shikhar Aug 19, 2016

Choose a reason for hiding this comment

pqueixalos commented Aug 22, 2016

ghost commented Aug 22, 2016

shikhar Aug 22, 2016

Choose a reason for hiding this comment

shikhar commented Aug 22, 2016

shikhar commented Aug 30, 2016

ewencp Aug 31, 2016

Choose a reason for hiding this comment

ewencp commented Aug 31, 2016

pqueixalos commented Aug 31, 2016

shikhar Sep 6, 2016

Choose a reason for hiding this comment

shikhar commented Sep 6, 2016

shikhar commented Sep 7, 2016

jmfn left a comment

Choose a reason for hiding this comment