Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-3234; Clarify minISR in documentation and auto-generate topic configuration docs #907

Closed
wants to merge 21 commits into from

Conversation

jjkoshy
Copy link
Contributor

@jjkoshy jjkoshy commented Feb 12, 2016

Fix design doc comments to clarify minISR and unclean leader election, switch topic configuration doc to be auto-generated.

<td>min.insync.replicas</td>
<td>1</td>
<td>min.insync.replicas</td>
<td>When a producer sets acks to "all", min.insync.replicas specifies the minimum number of replicas that must acknowledge a write for the write to be considered successful. If this minimum cannot be met, then the producer will raise an exception (either NotEnoughReplicas or NotEnoughReplicasAfterAppend).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I just realized that these "hand-written" configurations are more descriptive so we should probably additionally update the LogConfig class with these.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjkoshy Are you planning to update the descriptions in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - will do that sometime this week.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously has conflicts due to Becket's patch, but I'll leave this open for review given that the scope became a little larger (i.e., to consolidate broker and topic configurations). So not sure if everyone will be on board with this approach. cc a few others as well @junrao @becketqin to solicit thoughts/concerns.

@gwenshap
Copy link
Contributor

gwenshap commented Mar 4, 2016

LGTM

@gwenshap
Copy link
Contributor

gwenshap commented Mar 4, 2016

Ah, sorry - looks like there are merge conflicts. @jjkoshy, mind rebasing?

@jjkoshy
Copy link
Contributor Author

jjkoshy commented Mar 8, 2016

@gwenshap @becketqin one observation is that most of the log-specific configs in KafkaConfig are namespaced with log.. Although we did not do that for unclean.leader.election.enable, compression.type and min.insync.replicas I think we can deal with those later. However for the newer ones from KIP-3x (message.*) configs we should probably namespace those in KafkaConfig under log. before the next release.

@gwenshap
Copy link
Contributor

gwenshap commented Mar 8, 2016

Completely agree, @jjkoshy. Having a list of name conversions from cluster config to logs is embarrassing :)

@jjkoshy
Copy link
Contributor Author

jjkoshy commented Mar 10, 2016

Actually that turns out to be more involved than it seems. I was hoping to just prefix all the log configs in KafkaConfig with log and the LogConfig doc would simply say that the server defaults are the same but with a prefix of log. Unfortunately some of the base names are different so I would have to deprecate all of those. E.g., flush.ms in LogConfig flush.interval.ms in KafkaConfig; and even trickier: log.roll.jitter.{ms,hours} in KafkaConfig vs segment.jitter.ms

These changes just ensure that the server log configs are prefixed with log., but do not eliminate the need for a conversion table.

@ijuma
Copy link
Contributor

ijuma commented Mar 10, 2016

@jjkoshy I filed https://issues.apache.org/jira/browse/KAFKA-3373 for the KIP-31/32 configs. We should make a decision on that before 0.10.0.0 even if this particular PR is not ready. However, we could fix that as part of this PR if it is ready.

@becketqin
Copy link
Contributor

@jjkoshy Good point. I will add the 'log' prefix.

@jjkoshy
Copy link
Contributor Author

jjkoshy commented Mar 10, 2016

So here is a more elaborate/invasive patch - it basically deprecates all the non-conforming configs.

@jjkoshy
Copy link
Contributor Author

jjkoshy commented Mar 17, 2016

I would have liked this to go into 0.10.0 but I'm not sure it is feasible given that the latest patch needs a review and deprecates a number of configurations. I haven't done this in the current patch but if everyone agrees with the general approach then I would also emit a formal warning about the deprecated configurations if specified in the broker configuration or topic-level configuration (via the TopicCommand utilities).

@ijuma
Copy link
Contributor

ijuma commented Mar 17, 2016

@jjkoshy I agree that it's a bit tricky to get this in given the number of deprecations. I think it probably needs to be discussed in the mailing list. Would a KIP be appropriate?

@jjkoshy
Copy link
Contributor Author

jjkoshy commented Mar 17, 2016

Yes it should probably involve a (a rather boring) KIP

@ijuma
Copy link
Contributor

ijuma commented Mar 17, 2016

:)

asfgit pushed a commit that referenced this pull request Aug 7, 2016
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](https://github.com/apache/kafka/files/323901/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.

Author: James Cheng <jylcheng@yahoo.com>

Reviewers: Ismael Juma <ismael@juma.me.uk>, Joel Koshy <jjkoshy@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io>

Closes #1527 from wushujames/generate_topic_docs
efeg pushed a commit to efeg/kafka that referenced this pull request Jan 29, 2020
@vvcephei
Copy link
Contributor

vvcephei commented Feb 9, 2022

Hi @jjkoshy ,

It seems like this PR stalled. I'll close it out for now, but if you or anyone else want to resume this work, please feel free to re-open it (or start a new one)!

Thanks,
John

@vvcephei vvcephei closed this Feb 9, 2022
wcarlson5 pushed a commit to wcarlson5/kafka that referenced this pull request Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants