KAFKA-13809: Propagate full connector configuration to tasks in FileStream connectors#12450
KAFKA-13809: Propagate full connector configuration to tasks in FileStream connectors#12450C0urante merged 6 commits intoapache:trunkfrom
Conversation
|
@C0urante could you please take a look at this small change whenever you get a chance? |
|
Hi Yash! Thanks for the fix. It's been a long time coming that we patched the KAFKA-9228 gap with the file connectors. I'm wondering about some of the other changes, though. We use these connectors as examples for developers on our docs site, and the section at https://kafka.apache.org/32/documentation.html#connect_connectorexample includes several code snippets taken directly from them. Even though it's not strictly necessary to pull out the topic, filename, etc. in the Could you take a look at the docs and see if there's a way to address KAFKA-9228 (and, if you want, clean up the style of the connectors) and, at the same time, maintain the connectors' value as examples for developers? Feel free to tweak the docs in |
|
Hi Chris! Thanks for taking a look, and also for pointing out that the docs need to be updated - I'd completely missed that code snippets from these FileStream connectors were being used there. I've made some changes, could you PTAL? |
| static final ConfigDef CONFIG_DEF = new ConfigDef() | ||
| .define(FILE_CONFIG, Type.STRING, null, Importance.HIGH, "Source filename. If not specified, the standard input will be used") | ||
| .define(TOPIC_CONFIG, Type.LIST, Importance.HIGH, "The topic to publish data to") | ||
| .define(TOPIC_CONFIG, Type.STRING, ConfigDef.NO_DEFAULT_VALUE, new ConfigDef.NonEmptyString(), Importance.HIGH, "The topic to publish data to") |
There was a problem hiding this comment.
Not sure why this was defined as a LIST config when config values with more than one topic were anyway being rejected. I've updated this to be a STRING config and this should be backward compatible for config values that were valid previously.
| if (filename != null) | ||
| config.put(FILE_CONFIG, filename); | ||
| config.put(TOPIC_CONFIG, topic); | ||
| config.put(TASK_BATCH_SIZE_CONFIG, String.valueOf(batchSize)); |
There was a problem hiding this comment.
Removed the redundant parsing from String to int (in start()) and then back to String here; it's cleaner to use the helper methods from AbstractConfig directly in the task class.
C0urante
left a comment
C0urante
left a comment
There was a problem hiding this comment.
Thanks Yash! I want to clarify that I don't think we need the source code for the file connectors and the code snippets for them in our docs to completely match, but rather, that we should make sure that our docs remain valuable for connector developers and that any changes that we apply to the file connectors that may be beneficial to those developers be accurately reflected back in the docs.
… file config is an empty/blank string
C0urante
left a comment
C0urante
left a comment
There was a problem hiding this comment.
Thanks Yash--getting close!
2 participants
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)