Skip to content

Minor refactor in start script to remove Kafka-related leftovers - #2349

Open
danielgospodinow wants to merge 2 commits into
cruise-control-for-kafka:mainfrom
qbaware:refactor-start-script
Open

Minor refactor in start script to remove Kafka-related leftovers#2349
danielgospodinow wants to merge 2 commits into
cruise-control-for-kafka:mainfrom
qbaware:refactor-start-script

Conversation

@danielgospodinow

@danielgospodinow danielgospodinow commented Dec 17, 2025

Copy link
Copy Markdown

What

CC's start script defines variables with a KAFKA_ prefix which is incorrect. This is likely a leftover from copy-pasting Kafka start scripts. 😁

Looking at kafka-run-class.sh and kafka-server-start.sh further supports my claim. Lol.

Why

Might cause a confusion when reading the code, especially to an engineer that's new to the codebase.

Categorization

  • documentation
  • bugfix
  • new feature
  • refactor
  • security/CVE
  • other

Comment on lines -93 to -95
# If Cygwin is detected, LOG_DIR is converted to Windows format.
(( CYGWIN )) && LOG_DIR=$(cygpath --path --mixed "${LOG_DIR}")
KAFKA_LOG4J_OPTS="-Dkafka.logs.dir=$LOG_DIR $KAFKA_LOG4J_OPTS"

@danielgospodinow danielgospodinow Dec 17, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is the only line I've removed given that kafka.logs.dir is not part of Cruise Control. Everything else is just renaming.

@kyguy kyguy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

@kyguy

kyguy commented Dec 17, 2025

Copy link
Copy Markdown
Contributor

CircleCI failures are unrelated to the changes in this PR, they are due to some flaky tests which should be resolved by #2338.

@mimaison

Copy link
Copy Markdown
Contributor

This is a major operational change for anyone using Cruise Control. I expect a (large?) portion of users set some of these environment variables to tune Cruise Control and this is going to break their environments.

I can understand the desire to move away from Kafka names but it should be done in a compatible manner to give time for people to migrate.

I also wonder if names prefixed with something like CC_ would be better than the names used i this PR.

@danielgospodinow

Copy link
Copy Markdown
Author

I expect a (large?) portion of users set some of these environment variables to tune Cruise Control and this is going to break their environments.

True. Yet I think it's the right thing to do since the current names are confusing if you operate both CC and Kafka (which obviously is quite common), and it's anyway a simple change to adopt if something breaks.

I also wonder if names prefixed with something like CC_ would be better than the names used i this PR.

Sure, I'm fine with this suggestion too.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors kafka-cruise-control-start.sh to remove Kafka-prefixed environment variables (KAFKA_*) that were likely copy/pasted leftovers, making the script’s configuration surface less confusing for Cruise Control users.

Changes:

  • Renames several KAFKA_* env vars (e.g., KAFKA_OPTS, KAFKA_HEAP_OPTS, KAFKA_JMX_OPTS) to more generic equivalents (JVM_OPTS, HEAP_OPTS, JMX_OPTS, etc.).
  • Updates the Java launch command to use the renamed variables.
  • Removes some Kafka-specific log option wiring.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread kafka-cruise-control-start.sh
fi

# JMX port to use
if [ $JMX_PORT ]; then
Comment on lines 74 to 77
@@ -77,30 +77,26 @@
fi
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (5)

kafka-cruise-control-start.sh:109

  • This debug toggle still checks DEBUG. If you switch to a CC-specific variable name (e.g., CRUISE_CONTROL_DEBUG) to avoid collisions with common DEBUG env vars, this condition should be updated accordingly.
# Set Debug options if enabled
if [ "x$DEBUG" != "x" ]; then

kafka-cruise-control-start.sh:71

  • Using a generic DEBUG env var is likely to cause unintended Java debug enablement (many environments set DEBUG for unrelated tooling). Consider switching to a CC-specific name (e.g., CRUISE_CONTROL_DEBUG) and only mapping from the deprecated KAFKA_DEBUG for backward compatibility.

This issue also appears on line 107 of the same file.

# Backward compatibility for deprecated KAFKA_* env vars (remove after migration)
: "${JMX_OPTS:=${KAFKA_JMX_OPTS:-}}"
: "${LOG4J_OPTS:=${KAFKA_LOG4J_OPTS:-}}"
: "${JVM_OPTS:=${KAFKA_OPTS:-}}"
: "${HEAP_OPTS:=${KAFKA_HEAP_OPTS:-}}"
: "${JVM_PERFORMANCE_OPTS:=${KAFKA_JVM_PERFORMANCE_OPTS:-}}"
: "${GC_LOG_OPTS:=${KAFKA_GC_LOG_OPTS:-}}"
: "${DEBUG:=${KAFKA_DEBUG:-}}"

kafka-cruise-control-start.sh:86

  • The Cygwin LOG_DIR path conversion was removed. LOG_DIR is still used to build JVM args (e.g., -Xloggc:$LOG_DIR/...), so on Cygwin this can regress if Java expects Windows-style paths. Consider restoring the conversion independent of any Kafka-specific -Dkafka.logs.dir flag.
# Log directory to use
if [ "x$LOG_DIR" = "x" ]; then
  LOG_DIR="$base_dir/logs"
fi

kafka-cruise-control-start.sh:81

  • if [ $JMX_PORT ]; then will emit a test error when JMX_PORT is unset/empty. Use a proper non-empty check and quote the variable to avoid noisy stderr output and edge-case parsing issues.
# JMX port to use
if [  $JMX_PORT ]; then
  JMX_OPTS="$JMX_OPTS -Dcom.sun.management.jmxremote.port=$JMX_PORT "
fi

kafka-cruise-control-start.sh:148

  • The -f test should quote the path to avoid word-splitting/globbing issues (e.g., if base_dir contains spaces).
#Add jaas file to JVM_OPTS if present
if [ -f $base_dir/config/cruise_control_jaas.conf ]
then
  JVM_OPTS="-Djava.security.auth.login.config=$base_dir/config/cruise_control_jaas.conf $JVM_OPTS"
fi

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.

4 participants