CC-190: Deprecate the topic.index.map configuration option #151
Conversation
Signed-off-by: Arjun Satish <arjun@confluent.io>
There was a problem hiding this comment.
Thanks @wicknicks !
I have a few questions, which as usual I include them as comments.
docs/configuration_options.rst
Outdated
| @@ -96,6 +96,8 @@ Data Conversion | |||
| * Importance: low | |||
|
|
|||
| ``topic.index.map`` | |||
| This option is now deprecated. A future version may remove it completely. Please use single message transforms to map topic names to index names. | |||
There was a problem hiding this comment.
nit: A future version will remove it completely?
There was a problem hiding this comment.
nit: should we also offer some guidance w.r.t appropriate SMTs?
Please use single message transforms, such as RegexRouter or others, to map topic names ...
There was a problem hiding this comment.
Yeah, we can add the guidance. RegexRouter is the way to go.
Yes, we should be removing this option at a later, as RegexRouter subsumes all functionality of this configuration. Unless, there is a reason to deprecate something and leave it there.
There was a problem hiding this comment.
I feel that's a bit contradictory. Ones deprecates an option or a feature to signal that it will permanently be removed in the future for sure. If there's ambiguity regarding removal, then it shouldn't be marked as deprecated option.
| @@ -70,6 +70,11 @@ | |||
|
|
|||
| public static final String TYPE_NAME_CONFIG = "type.name"; | |||
| private static final String TYPE_NAME_DOC = "The Elasticsearch type name to use when indexing."; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
I believe we don't use javadoc style comments with config variables. Correct me if I'm wrong.
There was a problem hiding this comment.
This is mostly for reference. I suppose we could simply use // here. Thoughts?
There was a problem hiding this comment.
Indeed, we use // everywhere besides javadoc (and copyright header)
There was a problem hiding this comment.
An addition to my previous comment: The @deprecated annotation applies to fields as well (https://docs.oracle.com/javase/tutorial/java/annotations/predefined.html). Also, if we use Javadoc, the java compiler can report a warning when we use these fields in the code.
There was a problem hiding this comment.
My point was that we skip javadocs for all the configs, it wouldn't make much sense to add it only for the deprecated ones.
If you want to include the warning during build, feel free to add the @Deprecated annotation to the variable (outside a comment - javadoc or regular).
There was a problem hiding this comment.
We should use the @Deprecated annotation at least, and optionally the JavaDoc tag. In this case, I think the annotation would be sufficient.
There was a problem hiding this comment.
Got it. Removed the javadoc, and replaced it with a @Deprecated annotation.
| @@ -46,6 +46,9 @@ | |||
| private final Set<String> ignoreKeyTopics; | |||
| private final boolean ignoreSchema; | |||
| private final Set<String> ignoreSchemaTopics; | |||
| /** | |||
Signed-off-by: Arjun Satish <arjun@confluent.io>
Signed-off-by: Arjun Satish <arjun@confluent.io>
| @@ -96,6 +96,8 @@ Data Conversion | |||
| * Importance: low | |||
|
|
|||
| ``topic.index.map`` | |||
| This option is now deprecated. A future version may remove it completely. Please use single message transforms, such as RegexRouter, to map topic names to index names. | |||
|
|
|||
There was a problem hiding this comment.
The goal is that this .rst file can be largely generated by running ElasticsearchSinkConnectorConfig.main(...) and replacing nearly all of the file with the output (except the first few lines). To do that, we'd have to add this text to the documentation in the code. Once that's done, run the main mentioned above and paste into the docs/configuration_options.rst file as part of your commit.
There was a problem hiding this comment.
Thanks, @rhauch. Updated these lines using the main method.
…rConfig Signed-off-by: Arjun Satish <arjun@confluent.io>
Signed-off-by: Arjun Satish arjun@confluent.io