MINOR: Fix potential bug in LogConfig.getConfigValue and improve test coverage#7159
MINOR: Fix potential bug in LogConfig.getConfigValue and improve test coverage#7159ijuma merged 3 commits intoapache:trunkfrom
Conversation
… coverage LogConfig.getConfigValue would throw a NoSuchElementException if any log config was defined without a server default mapping. Added a unit test for `getConfigValue` and a sanity test for `toHtml`/`toRst`/`toEnrichedRst`, which were previously not exercised during the test suite.
| override def headers = List("Name", "Description", "Type", "Default", "Valid Values", ServerDefaultHeaderName, | ||
| "Importance").asJava | ||
|
|
||
| override def getConfigValue(key: ConfigKey, headerName: String): String = { |
There was a problem hiding this comment.
I wonder if we can come up with a better name. I was having a hard time understanding this patch because this is named so generically. Maybe something like docStringForHeader or something like that?
There was a problem hiding this comment.
Yeah, I agree that it's not a good name. I thought about renaming it too, but then decided to limit the scope. Happy to do it given your feedback.
There was a problem hiding this comment.
I guess this is grey area as far as compatibility is concerned since ConfigKey is public. Let's go ahead and leave it for now.
There was a problem hiding this comment.
getConfigValue is defined in ConfigDef and it's protected. However, ConfigDef is designed to be inherited from, so it could be considered a compatibility break to rename it. I agree with you that it's best to leave it for now.
|
retest this please |
|
One of the builds succeeded and the two others failed due to unrelated flaky tests. Started another build just in case. |
|
Thanks @ijuma. LGTM |
|
Two builds succeeded (although one reported a failure, it actually passed) and one failed with known flake (kafka.server.DescribeLogDirsRequestTest.testDescribeLogDirsRequest). Merging to master. |
… coverage (apache#7159) LogConfig.getConfigValue would throw a NoSuchElementException if any log config was defined without a server default mapping. Added a unit test for `getConfigValue` and a sanity test for `toHtml`/`toRst`/`toEnrichedRst`, which were previously not exercised during the test suite. Reviewers: Jason Gustafson <jason@confluent.io>, José Armando García Sancio <jsancio@users.noreply.github.com>
3 participants
LogConfig.getConfigValue would throw a NoSuchElementException if any log
config was defined without a server default mapping.
Added a unit test for
getConfigValueand a sanity test fortoHtml/toRst/toEnrichedRst, which were previously not exercised duringthe test suite.
Committer Checklist (excluded from commit message)