-
Notifications
You must be signed in to change notification settings - Fork 12
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
Stop removing _rev
from sink documents
#77
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, 1 nit
README.md
Outdated
transforms.ReplaceField.type=org.apache.kafka.connect.transforms.ReplaceField$Value | ||
transforms.ReplaceField.blacklist=_rev | ||
``` | ||
For more details, see [`ReplaceField`](https://docs.confluent.io/platform/current/connect/transforms/replacefield.html#replacefield) documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably point to the Apache Kafka documentation rather than the Confluent Platform.
Sadly it doesn't have a direct link to ReplaceField
but it is listed amongst the transforms in https://kafka.apache.org/31/documentation.html#connect_transforms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4e63cf6
README.md
Outdated
``` | ||
transforms=ReplaceField | ||
transforms.ReplaceField.type=org.apache.kafka.connect.transforms.ReplaceField$Value | ||
transforms.ReplaceField.blacklist=_rev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, should probably use exclude
instead of blacklist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4e63cf6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but we should use the non-deprecated exclude
Checklist
CHANGES.md
|CHANGELOG.md
) or test/build only changesDescription
fixes #72
Approach
Schema & API Changes
Security and Privacy
Testing
Monitoring and Logging