Skip to content

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

Open
danielgospodinow wants to merge 1 commit into
linkedin:mainfrom
qbaware:refactor-start-script
Open

Minor refactor in start script to remove Kafka-related leftovers#2349
danielgospodinow wants to merge 1 commit into
linkedin: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.

@@ -62,13 +62,13 @@ fi
shopt -u nullglob

# JMX settings
fi

# JMX port to use
if [ $JMX_PORT ]; then
Comment on lines 74 to 77
@@ -77,30 +77,26 @@ if [ "x$LOG_DIR" = "x" ]; then
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