KAFKA-3809: Auto-generate documentation for topic-level configuration #1527
Conversation
…evel config value that it inherits from.
…sion, min.insync.replicas, and message.timestamp.type
…each topic-level config inherits from.
|
Maybe @gwenshap. |
|
Thanks for picking this up by the way. :) |
|
Aside from copy/pasting the descriptions from the old docs into the new docs, I chose not to correct any of the descriptions in this, to make this a mostly mechanical change. I figured that a mechanical change could go in quickly, while new wording might require a couple iterations back and forth. However, I noticed a couple things that are wrong/could be improved. For example, the description for flush.messages wrongly says
when it already is a topic-level config. And the topic-level description of flush.messages is actually better than the broker level config, so it would make sense to update the broker level config. Should I make those improvements? I can make them in a separate checkin, since those might require more discussion. |
| val MinInSyncReplicasDoc = "If number of insync replicas drops below this number, we stop accepting writes with" + | ||
| " -1 (or all) required acks" | ||
| val DeleteRetentionMsDoc = "The amount of time to retain delete tombstone markers " + | ||
| "for <a href=\"#compaction\">log compacted</a> topics. This setting also gives a bound " + |
There was a problem hiding this comment.
Hmm, it'd be ideal if we didn't have to include links in docs. HTML is definitely the "native" output for docs for Kafka, but it's baking weird references that we don't have easy ways of checking into strings in the source code...
On the other hand, not sure I have a better suggestion.
There was a problem hiding this comment.
I'm not sure either.
On the one hand, it makes the docs clearer and better. It's really useful to click from the config section (which contains a simple one-line description) to the relevant section of the doc (which explains stuff in much more detail).
On the other hand, I agree that it's weird to have references in it that are not useful in isolation. The links will look weird and not make sense in the toRst() output, for example.
We could add the idea of a "footnote", which is added to the docs to say "for more info, go to see this section". It would be something on the ConfigDef, and the toHtmlString() method could use this footnote to append to the description, and the toRst() method would not. Hm, maybe it should be called an "HTML footnote"?
Not sure if it's worth building that code infrastructure.
There was a problem hiding this comment.
I chose to leave this as is, where the HTML docs have embedded links to other sections in the Kafka webpage. Hope that's okay.
|
@wushujames Thanks for the changes. I had some comments, but functionally this is great and fills in a bunch of important missing topic configs. Re: docs changes, feel free to leave them to a later patch (I think the only reason it may have made sense was that I had to go double check that they were, in fact, the same as the existing docs; once I realized that, reviewing that chunk of code wasn't really worthwhile). I think this is a great improvement since nobody wants to maintain this manually, just had a couple of possible refinements. |
| private final val serverDefaultConfigNames = mutable.Map[String, String]() | ||
|
|
||
| def define(name: String, defType: ConfigDef.Type, defaultValue: Object, validator: ConfigDef.Validator, | ||
| importance: ConfigDef.Importance, doc: String, serverDefaultConfigName: String) = { |
There was a problem hiding this comment.
We should include an explicit return type in public methods. And replace Object with Any. Applies to other define methods.
…er level description, not the other way around.
…lasses to override. Make LogConfig use that functionality to include additional columns.
|
@ewencp @ijuma I made all the suggested changes, except for the one about embedded HTML in the descriptions. This pull request was based off of trunk from 6 weeks ago. Do I need to merge trunk into the branch myself and update this PR, or will Github do it for me if/when you accept the pull request? Thanks. |
|
@wushujames, it's generally a good idea to merge trunk into the branch so that when someone (including yourself) tests your branch, they test the code as it will be once it's merged. |
|
It sounds like Github would do the merge if it can, but of course you're right that it's better for me to see it myself in order to test it. And Github would not be able to resolve merge conflicts, so I should do it myself anyway. I updated my branch to the latest trunk. I'm not sure if Jenkins is going to auto-rebuild it. |
|
Jenkins claimed that the PR failed in :streams:test, but the log didn't give any indication as to where. |
| val NumRecoveryThreadsPerDataDirDoc = "The number of threads per data directory to be used for log recovery at startup and flushing at shutdown" | ||
| val AutoCreateTopicsEnableDoc = "Enable auto creation of topic on the server" | ||
| val MinInSyncReplicasDoc = "define the minimum number of replicas in ISR needed to satisfy a produce request with acks=all (or -1)" | ||
| val MinInSyncReplicasDoc = LogConfig.MinInSyncReplicasDoc |
There was a problem hiding this comment.
This should probably be changed back as well.
There was a problem hiding this comment.
I did not have time to follow this conversation closely, but I think the reason I did the above in #907 was to consolidate all the docs in one place - i.e., I thought it would be better to have all in KafkaConfig or LogConfig, but not both. Anyway, @wushujames thanks a lot for picking this up :) and Ewen/Ismael for helping review!
…gConfig reference it. Fix indentation and style issues in LogConfigDef.getConfigValue().
|
LGTM. I think the only remaining issue is the HTML-specific links, but this is no worse than what we already had in the docs (and lack of ability to auto-generate). Will be nice to clean up later, but shouldn't block us from getting this in now. |
@ijuma said that it would make sense to split out this work from KAFKA-3234, since KAFKA-3234 had both a mechanical change (generating docs) as well as a change requiring discussion (deprecating/renaming config options).
@jjkoshy, I hope you don't mind that I took over this work. It's been 3 months since the last activity on KAFKA-3234, so I thought it would be okay to take over.
This work is essentially is the first 5-6 commits from Joel's #907. However, since I'm not very experienced with git, I didn't do a direct merge/rebase, but instead largely hand-merged it. I did some minor cleanup. All credit goes to Joel, all blame goes to me. :)
For reference, I attached the auto-generated configuration.html file (as a PDF, because github won't let me attache html).
configuration.pdf
This is my first time writing Scala, so let me know if there are any changes needed.
I don't know who is the right person to review this. @ijuma, can you help me redirect this to the appropriate person? Thanks.