{"action":"created","issue":{"url":"https://api.github.com/repos/confluentinc/kafka-connect-jdbc/issues/67","repository_url":"https://api.github.com/repos/confluentinc/kafka-connect-jdbc","labels_url":"https://api.github.com/repos/confluentinc/kafka-connect-jdbc/issues/67/labels{/name}","comments_url":"https://api.github.com/repos/confluentinc/kafka-connect-jdbc/issues/67/comments","events_url":"https://api.github.com/repos/confluentinc/kafka-connect-jdbc/issues/67/events","html_url":"https://github.com/confluentinc/kafka-connect-jdbc/pull/67","id":150223338,"number":67,"title":"Add support for single primary key column","user":{"login":"gfodor","id":220020,"avatar_url":"https://avatars.githubusercontent.com/u/220020?v=3","gravatar_id":"","url":"https://api.github.com/users/gfodor","html_url":"https://github.com/gfodor","followers_url":"https://api.github.com/users/gfodor/followers","following_url":"https://api.github.com/users/gfodor/following{/other_user}","gists_url":"https://api.github.com/users/gfodor/gists{/gist_id}","starred_url":"https://api.github.com/users/gfodor/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gfodor/subscriptions","organizations_url":"https://api.github.com/users/gfodor/orgs","repos_url":"https://api.github.com/users/gfodor/repos","events_url":"https://api.github.com/users/gfodor/events{/privacy}","received_events_url":"https://api.github.com/users/gfodor/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":14,"created_at":"2016-04-22T00:02:55Z","updated_at":"2016-08-08T03:38:34Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/confluentinc/kafka-connect-jdbc/pulls/67","html_url":"https://github.com/confluentinc/kafka-connect-jdbc/pull/67","diff_url":"https://github.com/confluentinc/kafka-connect-jdbc/pull/67.diff","patch_url":"https://github.com/confluentinc/kafka-connect-jdbc/pull/67.patch"},"body":"Here's an attempt at adding support for the JDBC connect driver to send messages to Kafka with the primary key value for a table as the message key. I was not sure of the proper way to support composite primary keys, so the patch only adds support for tables that have a single primary key.\r\n\r\nThis is obviously a WIP, I wanted to get feedback on the composite primary key issue as well as the proper level of configurability for this feature. This seems like something that should be on by default, since it's likely the behavior of least surprise, but backwards-compatibility concerns obviously are there to consider as well.\r\n\r\nThis PR also adds the `incrementing.column.use_primary_key` option -- this is an option which will force the connector to assume the primary key is an incrementing column. For our use-case, this was a necessary addition, since our primary key columns are generated via a stored procedure (which generates an auto incremented value) but are not reported by the JDBC driver as auto incrementing, nor are all the primary key columns named the same so just setting `incrementing.column.name` cannot be used."},"comment":{"url":"https://api.github.com/repos/confluentinc/kafka-connect-jdbc/issues/comments/238135333","html_url":"https://github.com/confluentinc/kafka-connect-jdbc/pull/67#issuecomment-238135333","issue_url":"https://api.github.com/repos/confluentinc/kafka-connect-jdbc/issues/67","id":238135333,"user":{"login":"gfodor","id":220020,"avatar_url":"https://avatars.githubusercontent.com/u/220020?v=3","gravatar_id":"","url":"https://api.github.com/users/gfodor","html_url":"https://github.com/gfodor","followers_url":"https://api.github.com/users/gfodor/followers","following_url":"https://api.github.com/users/gfodor/following{/other_user}","gists_url":"https://api.github.com/users/gfodor/gists{/gist_id}","starred_url":"https://api.github.com/users/gfodor/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gfodor/subscriptions","organizations_url":"https://api.github.com/users/gfodor/orgs","repos_url":"https://api.github.com/users/gfodor/repos","events_url":"https://api.github.com/users/gfodor/events{/privacy}","received_events_url":"https://api.github.com/users/gfodor/received_events","type":"User","site_admin":false},"created_at":"2016-08-08T03:38:34Z","updated_at":"2016-08-08T03:38:34Z","body":"Ugh, github swallowed all my comments on the PR due to the merge commit. I incorporated most of the feedback however there are still two sticking points:\r\n\r\nFirst is that the configuration option I added for `incrementing.column.use.primary.key` likely should not be rolled into the `mode` since it seems to be an orthogonal concern. Currently there are two ways an incremently column is determined, either via the AUTO INCREMENT metadata or by a fixed name via `incrementing.column.name`, and this is a third. (If we were to roll this into the `mode`, we'd need two new modes, one for the `incrementing` mode and one for the `timestamp+incrementing` mode, which should show that this is a separate concern.) The current configuration is a bit strange, a cleaner, but non-backwards-compatible one would be to introduce `incrementing.column.type` which could be `auto_increment` (default), `primary_key`, or `fixed`, and `incrementing.column.name` is allowed if and only if `fixed` is specified.\r\n\r\nThe second set of unresolved issues in the PR are around error cases:\r\n- I'd argue that a WARN is the proper logging for when a composite primary key is discovered in general, since it can fall back on the behavior of a table with no primary key, and not specify keys for the records. This may be surprising to users, but seems better than preventing users from syncing composite primary keyed tables, so a WARN seems to suffice. (I can update the log statement.)\r\n- However if the user specifies that the primary key should be used for the incrementing column, but a singular primary key column is not found (either due to their not being one, or it being composite), this should be an error.\r\n- Finally the question was brought up of what to do when there is a NULL value in a known primary key column. It seems the least surprising thing to do here is to still send that record to Kafka with a null key, but the alternative would be to emit an error.\r\n\r\nAlso I might have missed it but I didn't see any comments on the fact that this patch changes the semantics of records going to Kafka for any tables that have primary key columns: before this patch, they will have null keys, and after it they will have the key of the primary key column. If there is flexibility on backwards compatibility there, I feel strongly this is a positive change and is probably the expected behavior, but I wanted to make sure you're aware of the fact that users may run into issues where (for example) they are relying upon the random partitioning currently happening due to the null Kafka keys being sent for all records, regardless of their primary key."}}
{"actor":{"display_login":"gfodor"}}