MINOR: clean up and add getters to config #445
Conversation
Signed-off-by: Lev Zemlyanov <lev@confluent.io>
| ).define( | ||
| CONNECTION_PASSWORD_CONFIG, | ||
| Type.PASSWORD, | ||
| CONNECTION_PASSWORD_DEFAULT, |
There was a problem hiding this comment.
Should this have a default? or should it be defined with Importance.HIGH as a required config?
There was a problem hiding this comment.
this is not a required config
There was a problem hiding this comment.
Thanks for the cleanup @levzem! Few questions.
One note for ES as a future improvement (not this PR) would be more integration test coverage around edge cases/negative paths, it seems to be behind compared to some of our other connectors.
| return getString(TYPE_NAME_CONFIG); | ||
| } | ||
|
|
||
| public Set<String> urls() { |
There was a problem hiding this comment.
It would be great if we could use a more descriptive name for this getter, ex. connectionUrls. Let's also make sure getList doesn't eliminate duplicates already.
| return getBoolean(COMPACT_MAP_ENTRIES_CONFIG); | ||
| } | ||
|
|
||
| private Map<String, String> parseMapConfig(List<String> values) { |
There was a problem hiding this comment.
What is this used for in this repo? First time I see this logic in connector configs.
There was a problem hiding this comment.
TOPIC_INDEX_MAP_CONFIG / topicIndexMap()
Signed-off-by: Lev Zemlyanov <lev@confluent.io>
Signed-off-by: Lev Zemlyanov lev@confluent.io
Problem
connector does not use config getters which is not the best practice and ends up with some clunky code
Solution
add config getters and reorganize config class
Does this solution apply anywhere else?
If yes, where?
Test Strategy
ensure all previous tests pass
Testing done:
Release Plan
backport to
5.0.xto reduce pint merge conflicts in future PRs