-
Notifications
You must be signed in to change notification settings - Fork 106
Add support for change tracking on table creation #1212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
@sfc-gh-lkucharski could you please give this a review? |
Hi, thank you for the contribution. A few things to keep in mind
|
src/main/java/com/snowflake/kafka/connector/internal/SnowflakeConnectionServiceV1.java
Outdated
Show resolved
Hide resolved
Resolved conflicts, applied the suggested change. We're happy for this to go into 4.X as we're already using |
I'll take a look tomorrow |
|
@sfc-gh-lkucharski did you get the chance to review this? Just resolved the more recent merge conflicts. I don't have the credentials on my fork for this repo to run all the tests so would be great to get this branch pulled into the main repo to run the tests in a pull request from there. |
Thank you again for this effort. Can you explain what you mean by "pulled into the main repo"?. Your pull request looks like a nice to have feature. Currently, I think it's the worst possible time to work on this, as we are undergoing major refactoring on master for version 4 of the connector. We are rearranging classes and deleting content. Currently, the master doesn't even support automatic table creation. Therefore, this feature is not functional temporarily. Do you think we can revisit this in two weeks? I think internally I have like four pull requests waiting to be merged. I wouldn't want you to resolve conflicts on every PR we merge. I would gladly let you know if the code gets stable enough so that you won't waste your time resolving conflicts all the time. What do you think? |
A bunch of Checks failed with the error It just means that my fork does not have the credentials needed to run them. This is expected and not a problem on my side. Just allows the checks to run before this can be merged.
Yes, that's totally fine. Just let me know when your changes are stable, and I'll update this pull request to resolve any conflicts. |
|
Hi @momirza, hope you're doing well. I'm just letting you know that the master branch is now pretty stable. If you still need that feature, please resolve the conflicts and fix the tests. Best, Lukas |
Great thanks! I'll have a look later next week :) |
Related to #343
Overview
SNOW-XXXXX
Pre-review checklist
snowflake.enable.change.tracking.Yes- Added end to end and Unit Tests.No- Suggest why it is not param protectedEmail for CLA: mo.mirza@iwoca.co.uk