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

Fix PartitionSizeAnomalyFinder, to be able to handle custom SELF_HEALING_PARTITION_SIZE_THRESHOLD_MB value #2212

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

k0b3rIT
Copy link

@k0b3rIT k0b3rIT commented Oct 24, 2024

Summary

  1. Why: I ran into the following exception when I tried to customize the SELF_HEALING_PARTITION_SIZE_THRESHOLD_MB in PartitionSizeAnomalyFinder
java.lang.ClassCastException: class java.lang.String cannot be cast to class java.lang.Integer (java.lang.String and java.lang.Integer are in module java.base of loader 'bootstrap')
	at com.linkedin.kafka.cruisecontrol.detector.PartitionSizeAnomalyFinder.configure(PartitionSizeAnomalyFinder.java:111) ~[classes/:?]
	at com.linkedin.kafka.cruisecontrol.config.KafkaCruiseControlConfig.getConfiguredInstances(KafkaCruiseControlConfig.java:119) ~[classes/:?]
	at com.linkedin.kafka.cruisecontrol.detector.TopicAnomalyDetector.<init>(TopicAnomalyDetector.java:32) ~[classes/:?]
	at com.linkedin.kafka.cruisecontrol.detector.AnomalyDetectorManager.<init>(AnomalyDetectorManager.java:120) ~[classes/:?]

  1. What:
    I have fixed the config reader code to parse the integer value

Expected Behavior

Be able to reconfigure the SELF_HEALING_PARTITION_SIZE_THRESHOLD_MB

Actual Behavior

I cant reconfigure the SELF_HEALING_PARTITION_SIZE_THRESHOLD_MB

Steps to Reproduce

  1. Enable the PartitionSizeAnomaly finder topic.anomaly.finder.class=com.linkedin.kafka.cruisecontrol.detector.PartitionSizeAnomalyFinder
  2. Provide a custom value for self.healing.partition.size.threshold.mb=1024
  3. Start CC

Known Workarounds

NO

Categorization

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

@k0b3rIT k0b3rIT force-pushed the fix_partitionsizeanomalyfinder branch from ec3db67 to 195e35e Compare October 24, 2024 05:23
@viktorsomogyi
Copy link
Contributor

@mhratson would you please review this?

Comment on lines 112 to 115
_partitionSizeThresholdInMb = DEFAULT_SELF_HEALING_PARTITION_SIZE_THRESHOLD_MB;
if (partitionSizeThresholdStr != null) {
_partitionSizeThresholdInMb = Integer.parseInt(partitionSizeThresholdStr.trim());
}

Choose a reason for hiding this comment

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

Suggested change
_partitionSizeThresholdInMb = DEFAULT_SELF_HEALING_PARTITION_SIZE_THRESHOLD_MB;
if (partitionSizeThresholdStr != null) {
_partitionSizeThresholdInMb = Integer.parseInt(partitionSizeThresholdStr.trim());
}
_partitionSizeThresholdInMb = partitionSizeThreshold == null ? DEFAULT_SELF_HEALING_PARTITION_SIZE_THRESHOLD_MB
: Integer.parseInt(partitionSizeThresholdStr.trim());

Preserves style, similar to TopicReplicationFactorAnomalyFinder.
Personally I'd also remove the trim() to maintain the same behaviour as other numerical configs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@k0b3rIT please apply the suggestion

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I have applied it!

@Manicben
Copy link

Thanks for raising this, we've ran into the same issue and I was going to raise a PR of my own. Would be great to get this reviewed and merged 🙏

@k0b3rIT k0b3rIT force-pushed the fix_partitionsizeanomalyfinder branch from 195e35e to 58429e3 Compare October 28, 2024 09:21
@viktorsomogyi
Copy link
Contributor

@mhratson would you please review this?

@viktorsomogyi
Copy link
Contributor

@CCisGG would you please review this if you have some time?

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