-
Notifications
You must be signed in to change notification settings - Fork 955
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
Update doc for CP 3.0 #73
Conversation
|
||
* Type: long | ||
* Default: 0 | ||
* Importance: low |
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.
Hmm, I don't think this was caught during the review when this option was added -- this was an oversight in the initial implementation and is probably pretty important. Should we raise the importance level here (at least to medium, maybe to high)?
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.
I think we should raise it to high.
@Ishiihara A couple of thoughts:
|
…addtional documentation
@ewencp Made suggested changes. PTAL. Thanks! |
Note that all incremental query modes that use certain columns to detect changes will require | ||
indexes on those columns to efficiently perform the queries. | ||
|
||
For incremental query modes that use timestamps, the JDBC connector uses a configuration |
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.
nit: maybe "the wait after" -> "the waiting period after". Same in following sentence
@Ishiihara minor comment on text. For the reorganization, it doesn't seem like it quite follows the pattern of other projects. Should some of the sections like "Configuration Options" and "Schema Evolution" be on separate pages? I'm not really sure here since we don't match all the introductory sections either.... |
@@ -1,359 +1,10 @@ | |||
.. Kafka Connect JDBC documentation master file |
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.
It will be helpful to have a reference to link to here like many other sections do.
@ewencp Addressed the comments. Made some minor reorganization. PTAL. Thanks! |
@Ishiihara LGTM, but I think we now want to do this against 3.x and then have it merged back to master. You can just submit a new PR against 3.x and merge immediately. |
No description provided.