-
Notifications
You must be signed in to change notification settings - Fork 5
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
Documentation and Config updates #142
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.
From a high level this looks good - will wait on a review from someone more familiar with the changes before I merge it :)
README.md
Outdated
| client.loglevel | Sets the verbosity of logging. | *Integer* | 0 | | ||
| Property | Description | Type | Default | | ||
|-----------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------|-----------|---------| | ||
| message.name | Ably message name to publish. Can be a pattern, as per `channel` above. | *String* | | |
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 believe the context for patterns is in Dynamic Channel Configuration
, with channel just directing to that section, so we could just direct to that section instead.
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.
Done in aba1da6
| Property | Description | Type | Default | | ||
|-----------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------|-----------|---------| | ||
| message.name | Ably message name to publish. Can be a pattern, as per `channel` above. | *String* | | | ||
| batchExecutionThreadPoolSize | The maximum number of parallel outgoing REST API requests to publish content to Ably | *Integer* | 10 | |
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.
For all of these limits are there any additional maximums and minimums to what these values can be?
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.
For batch publishing, you'd be limited by the maximum payload size limits here:
https://ably.com/docs/api/rest-api#batch-publish
So it would be a case-by-case basis
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.
Added a line about the limits: 162248a
@@ -154,12 +136,12 @@ public class ChannelSinkConnectorConfig extends AbstractConfig { | |||
public static final String BATCH_EXECUTION_THREAD_POOL_SIZE_DEFAULT = "10"; | |||
|
|||
public static final String BATCH_EXECUTION_MAX_BUFFER_SIZE = "batchExecutionMaxBufferSize"; | |||
public static final String BATCH_EXECUTION_MAX_BUFFER_SIZE_DEFAULT = "1000"; | |||
public static final String BATCH_EXECUTION_MAX_BUFFER_SIZE_DEFAULT = "100"; |
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.
This is 100, but in https://github.com/ably/kafka-connect-ably/pull/142/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R223 (which I believe is what this is the default value for) it's still 1000.
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.
Done in 83672a0
private static final String BATCH_EXECUTION_MAX_BUFFER_SIZE_DOC = "Size of the buffer, records " + | ||
"are buffered or chunked before calling the Ably Batch REST API"; | ||
|
||
public static final String BATCH_EXECUTION_MAX_BUFFER_DELAY_MS = "batchExecutionMaxBufferSizeMs"; | ||
public static final String BATCH_EXECUTION_MAX_BUFFER_DELAY_MS_DEFAULT = "5000"; | ||
public static final String BATCH_EXECUTION_MAX_BUFFER_DELAY_MS_DEFAULT = "100"; |
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.
This is 100, but in https://github.com/ably/kafka-connect-ably/pull/142/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R224 (which I believe this is the default value for) it's still 5000.
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.
Done in 83672a0
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, thanks!
This PR finished up the V3 release by adding docs for new functionality, removing irrelevant and superseded docs and config and fixing the local docker build.
@paddybyers and @tomczoink -- I added you because the README changes give you a decent summary of what's new and thought it might be good for you to review that.