Kafka: Settings and KRaft support #224611
Conversation
|
Sort of fixes #203987 in a very un-opinionated way (by letting the settings be agnostic to the type of cluster, for now) |
srhb
force-pushed
the
kafka-settings-module
branch
from
777fced
to
cd1800b
Compare
srhb
force-pushed
the
kafka-settings-module
branch
from
b583763
to
3ac906c
Compare
|
Okay, should be ready for review! I'm especially curious if the |
|
@ofborg test kafka.kafka kafka.kafka_kraft |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/2038 |
srhb
force-pushed
the
kafka-settings-module
branch
from
3ac906c
to
bee88f2
Compare
|
@roberth Thanks! That's exactly what I need. 😄 |
|
@ofborg test kafka.kafka kafka.kafka_kraft |
There was a problem hiding this comment.
(Note: I work in the same team as Sarah)
Looking good. Have tested different configs - works fine.
´mkRenamedOptionModule´ part works fine - ala:
trace: warning: The option services.apache-kafka.brokerId' defined in /home/ers/work/deployments/deployments/kafka/kafka.nix' has been renamed to services.apache-kafka.settings.""broker.id""'.`
Running tests from nixpkgs-review pr 224611 works fine.
Here at DBC we have tested the module with both zookeeper and kraft kafka clusters - works fine.
Approved.
Note: New options from manual:
`services.apache-kafka.enable
Whether to enable Apache Kafka event streaming broker.
Type: boolean
Default: false
Example: true
Declared by:
<nixpkgs/nixos/modules/services/misc/apache-kafka.nix>
services.apache-kafka.package
The kafka package to use
Type: package
Default: pkgs.apacheKafka
Declared by:
<nixpkgs/nixos/modules/services/misc/apache-kafka.nix>
services.apache-kafka.clusterId
KRaft mode ClusterId used for formatting log directories. Can be generated with kafka-storage.sh random-uuid
Type: null or string
Default: null
Declared by:
<nixpkgs/nixos/modules/services/misc/apache-kafka.nix>
services.apache-kafka.formatLogDirs
Whether to format log dirs in KRaft mode if all log dirs are unformatted, ie. they contain no meta.properties.
Type: boolean
Default: false
Declared by:
<nixpkgs/nixos/modules/services/misc/apache-kafka.nix>
services.apache-kafka.formatLogDirsIgnoreFormatted
Whether to ignore already formatted log dirs when formatting log dirs, instead of failing. Useful when replacing or adding disks.
Type: boolean
Default: false
Declared by:
<nixpkgs/nixos/modules/services/misc/apache-kafka.nix>
services.apache-kafka.jre
The JRE with which to run Kafka
Type: package
Default: pkgs.apacheKafka.passthru.jre
Declared by:
<nixpkgs/nixos/modules/services/misc/apache-kafka.nix>
services.apache-kafka.jvmOptions
Extra command line options for the JVM running Kafka.
Type: list of string
Default: [ ]
Example:
[
"-Djava.net.preferIPv4Stack=true"
"-Dcom.sun.management.jmxremote"
"-Dcom.sun.management.jmxremote.local.only=true"
]
Declared by:
<nixpkgs/nixos/modules/services/misc/apache-kafka.nix>
services.apache-kafka.log4jProperties
Kafka log4j property configuration.
Type: strings concatenated with “\n”
Default:
''
log4j.rootLogger=INFO, stdout
log4j.appender.stdout=org.apache.log4j.ConsoleAppender
log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
log4j.appender.stdout.layout.ConversionPattern=[%d] %p %m (%c)%n
''
Declared by:
<nixpkgs/nixos/modules/services/misc/apache-kafka.nix>
services.apache-kafka.serverProperties
Literal server.properties content. Will be appended to the config after services.apache-kafka.settings
Type: strings concatenated with “\n”
Default: ""
Declared by:
<nixpkgs/nixos/modules/services/misc/apache-kafka.nix>
services.apache-kafka.settings
Kafka broker configuration server.properties.
Note that .properties files contain mappings from string to string. Keys with dots are NOT represented by nested attrs in these settings, but instead as quoted strings (ie. settings."broker.id", NOT settings.broker.id).
Type: lazy attribute set of (null or boolean or signed integer or string or list of (boolean or signed integer or string))
Declared by:
<nixpkgs/nixos/modules/services/misc/apache-kafka.nix>
services.apache-kafka.settings."broker.id"
Broker ID. -1 or null to auto-allocate in zookeeper mode.
Type: null or signed integer
Default: null
Declared by:
<nixpkgs/nixos/modules/services/misc/apache-kafka.nix>
services.apache-kafka.settings.listeners
Kafka Listener List. See listeners.
Type: list of string
Default:
[
"PLAINTEXT://localhost:9092"
]
Declared by:
<nixpkgs/nixos/modules/services/misc/apache-kafka.nix>
services.apache-kafka.settings."log.dirs"
Log file directories.
Type: list of path
Declared by:
<nixpkgs/nixos/modules/services/misc/apache-kafka.nix> `
|
I'm refreshing this for another attempt. Last time I failed to get approval from someone who isn't on my team, which felt a bit too nepotism-adjacent for my taste. I'll try once more and/or lower my standards. 😅 |
srhb
force-pushed
the
kafka-settings-module
branch
3 times, most recently
from
1df74fe
to
fd178f8
Compare
|
@ofborg test kafka.kafka kafka.kafka_kraft |
|
Think this is back on track. I did:
I would appreciate a review from someone outside of my org. :) |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/2838 |
There was a problem hiding this comment.
Great PR!
Have a few superficial suggestions, but the best thing you could do is to start a section in the manual about Kafka on NixOS.
I have no domain knowledge about Kafka to speak of, so I'll have to trust you to get that part right, and it seems that you have the experience.
| @@ -264,6 +264,11 @@ | |||
|
|
|||
| - The binary of the package `cloud-sql-proxy` has changed from `cloud_sql_proxy` to `cloud-sql-proxy`. | |||
|
|
|||
| - The module `services.apache-kafka` was largely rewritten and has certain breaking changes. To be precise, this means that the following things have changed: | |||
|
|
|||
| - Most settings has been migrated under [services.apache-kafka.settings](#opt-services.apache-kafka.settings) which is an attribute-set that will be converted into Apache Kafka's .properties config format. This means that the [Broker Configs documentation](https://kafka.apache.org/documentation/#brokerconfigs) should be followed closely and that the module is less opinionated. Note that dotted options in the upstream docs do _not_ correspond to nested Nix attrsets, but instead as quoted top level `settings` attributes, as in `services.apache-kafka.settings."broker.id"`, NOT `services.apache-kafka.settings.broker.id`. Care should be taken, especially when migrating clusters from the old module, to ensure that the same intended configuration is reproduced faithfully via `settings` alone. | |||
There was a problem hiding this comment.
tip: Having one sentence per line helps with reviews and tends to make diffs smaller (without having to do manual reflowing when changing a sentence).
There was a problem hiding this comment.
Care should be taken,
How do I review my new file?
It would be nice to be able to be able to do nix-build <nixpkgs/nixos> -A config.services.apache-kafka.configFiles.serverProperties (assuming a "locally" deployed evaluation, which users can reasonably extrapolate from).
This would involve turning serverConfig binding into aforementioned option.
I think all three/four elements are relevant in that name
- config as opposed to paths for stateful things
- files to indicate that we're talking about a file path, and a file that's built with a derivation, as opposed to an attrset.
- Someone could also use this to provide a file path to a mutable location if that's what they're into. I'd say let them use
mkForceto achieve that, because I assume their use case is somewhat unsupported.
- Someone could also use this to provide a file path to a mutable location if that's what they're into. I'd say let them use
- serverProperties the name, reformatted for simplicity
There was a problem hiding this comment.
I have tried to cut back on the release note and I have implemented the new option as you suggested, enforcing a mkForce for the configFiles.serverProperties by setting it in config.
I have added a note to the manual on how to build this, as you said.
| - The module `services.apache-kafka` was largely rewritten and has certain breaking changes. To be precise, this means that the following things have changed: | ||
|
|
||
| - Most settings has been migrated under [services.apache-kafka.settings](#opt-services.apache-kafka.settings) which is an attribute-set that will be converted into Apache Kafka's .properties config format. This means that the [Broker Configs documentation](https://kafka.apache.org/documentation/#brokerconfigs) should be followed closely and that the module is less opinionated. Note that dotted options in the upstream docs do _not_ correspond to nested Nix attrsets, but instead as quoted top level `settings` attributes, as in `services.apache-kafka.settings."broker.id"`, NOT `services.apache-kafka.settings.broker.id`. Care should be taken, especially when migrating clusters from the old module, to ensure that the same intended configuration is reproduced faithfully via `settings` alone. | ||
| - By virtue of being less opinionated, it is now possible to use the module to run Apache Kafka in KRaft mode instead of Zookeeper mode. New options have been provided to assist with formatting `"log.dirs"` for KRaft mode clusters: [services.apache-kafka.clusterId](#opt-services.apache-kafka.clusterId), [services.apache-kafka.formatLogDirs](#opt-services.apache-kafka.formatLogDirs), [services.apache-kafka.formatLogDirsIgnoreFormatted](#opt-services.apache-kafka.formatLogDirsIgnoreFormatted). |
There was a problem hiding this comment.
The details of this should be in the Configuration chapter of the NixOS manual, because it's a thing you could do in the upcoming release, or the release after, etc - independently.
A short section is infinitely better than nothing at all, because then it can grow.
There was a problem hiding this comment.
I've done this, please take a look at the manual section to see if this looks better. It's a bit wordy, but maybe that's fine 😓
| cat $settings > $out | ||
| cat $propertiesTextPath >> $out |
There was a problem hiding this comment.
Was going to suggest that a single cat is sufficient and that it was originally for concatenation, but you could use concatText instead.
There was a problem hiding this comment.
No longer relevant as I dropped support for joining anything to settings as requested.
| (mkRemovedOptionModule [ "services" "apache-kafka" "port" ] | ||
| "Please see services.apache-kafka.settings.listeners and its documentation instead") | ||
| (mkRemovedOptionModule [ "services" "apache-kafka" "hostname" ] | ||
| "Please see services.apache-kafka.settings.listeners and its documentation instead") | ||
| (mkRemovedOptionModule [ "services" "apache-kafka" "extraProperties" ] | ||
| "Please see services.apache-kafka.settings and its documentation instead") |
There was a problem hiding this comment.
Will all users encounter these errors?
It would be great to tell them that they have to perform a non-trivial migration, where they should manually verify the correctness of their config. (Or am I reading too much into your earlier suggestion that it's less opinionated and therefore breaking? That was some inference on my part.)
A let binding would be appropriate for that piece of text then.
There was a problem hiding this comment.
I'm not sure I understood this review comment, sorry. Did you notice that each message differs where I have referred to the new settings-based option instead? Or what did you intend with the let-binding?
|
@roberth Thank you for the thorough review! I've tacked on commits at the end marked "wip" (that I mean to squash in later) to hopefully aid in re-review, and I'll respond to each review thread individually. |
srhb
force-pushed
the
kafka-settings-module
branch
from
5ee36ef
to
a3c1160
Compare
|
Gente re-ping. Would love to get approval and get this in before the window closes. C: |
- Use lazyAttrs (for config references) settings for main server.properties. - Drop dangerous default for "log.dirs" - Drop apache-kafka homedir; unused and confusing - Support formatting kraft logdirs
srhb
force-pushed
the
kafka-settings-module
branch
from
a3c1160
to
cfe3ca1
Compare
|
Well, I did the rebase, hopefully it's still clear how I addressed the review comments. |
There was a problem hiding this comment.
LGTM. Still relying on your and eskytthe's testing.
TIL this package does have a maintainer, @ragnard, but they haven't interacted for years, so I'm not going to further delay these changes.
@srhb Would you like to be listed as maintainer in the package?
|
Successfully created backport PR for |
|
Git push to origin failed for release-23.11 with exitcode 1 |
Description of changes
Motivation: Support more possible configurations, specifically KRaft (and various migration modes) without having to bail out and write server.properties out by hand.
settingsDiscussion points:
I wrote this module by converting our existing in-house clusters (we have both KRaft and zookeeper clusters) and trying to find a sensible set of options that works for both. While doing this, I needed config references across our deployments, which is why I chose
lazyAttrsOf. I could have written out more options by hand, but frankly it seems completely arbitrary which options users might rely on references. Let me know if this is a poor choice.I chose to "shoehorn" in a dropped default for logDirs. I found the existing default scary, and since users are presented with warnings/errors and hopefully a read of the release notes, now seems like a good time to make them actively deliberate their values.
I dropped the homedir for the apache-kafka users since, to my knowledge, it is never used, and the value choice of "first logdir" seems pretty arbitrary to me. /var/empty should be the new default, and it's readonly.
KRaft has some rather annoying requirements of "formatting" log dirs (provisioning certain metadata files) out-of-band of the server process so I've introduced regular NixOS options to support doing this automatically in
preStart. I'm hoping to engage upstream to get rid of this requirement, which seems like a step in the wrong direction for Kafka, but here we are. See KIP-785 and related discussion.Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)