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

[SAI-5330] Set the default value of priorityBasedEnabled to false #245

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

hiteshk25
Copy link
Collaborator

SAI-5330 Fixed the null pointer exception by setting the default value. (it happens when you don't set priorityBasedEnabled)

Copy link
Collaborator

@patsonluk patsonluk left a comment

Choose a reason for hiding this comment

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

Just several thoughts:

  1. Looks like these changes are in 9_7 only, are we going to merge them to 9.x later? cause usually we start with 9x first and cherry pick to 9_7 (tho i do see some existing changes on RateLimiterPayload are in 9_7 only it seems)
  2. For consistencies, perhaps we need null check for field enabled at here as well otherwise it will throw NPE if not defined. As the @JsonProperty definition of enabled was not marked as required, and the doc implies that it is not required - since there's a default (as false).
  3. This one is kinda minor, but other fields appear to not define the default values in the bean itself (RateLimiterPayload) but at the actual usage of the field. The RateLimiterPayload seems to be a pure bean that makes no assumption of default values (as in other fields). That being said, setting a default in RateLimiterPayload is likely not a big deal 🤷🏼 (subtle difference in the copy and equals method, it could in some scenario write a false value back to the json if it was undefined?)

@hiteshk25
Copy link
Collaborator Author

  1. Good catch. I will merge all these change in branch_9x.
  2. Updated. I saw it but didn't want to change other code.
  3. Bean thing is weird(null vs default value). But now equal/copy should be ok. Those are mainly used for dynamic change. whether config is changed or not.

@hiteshk25 hiteshk25 merged commit 86d38c9 into fs/branch_9_7 Jan 7, 2025
3 checks passed
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.

2 participants