Skip to content

Conversation

LZD-PratyushBhatt
Copy link

@LZD-PratyushBhatt LZD-PratyushBhatt commented Aug 14, 2025

Issues

  • My PR addresses the following Helix issues and references them in the PR description:

(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)

Description

  • Here are some details about my PR, including screenshots of any UI changes:

(Write a concise description including what, why, how)
Currently EV only gets updated only as part of these events CurrentStateChange, LiveInstanceChange, PeriodicRebalance, OnDemandRebalance and ControllerChange.
It doesnt get updated for IdealState changes. When a change is applied to the ideal_state of a resource, it’s expected that the EV reflect the changes. In cases of changes in configs like rebalancer algo, or min_active_replica, the change might not result in an assignment change, and Helix just forgets to update the EV to reflect these .

This change proposes updating the EV when IS changes. Keep EV an honest reflection of the state of the resource, when a resource-change event occurs. if it leads to change in assignment or not .
For updating EV, from IS we only take SimpleFields and putall the simple configs from IS to EV.

Tests

New test class TestExternalViewComputeOnIdealStateChange

  • The following tests are written for this issue:

(List the names of added unit/integration tests)

  • The following is the result of the "mvn test" command on the appropriate module:

(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)

Changes that Break Backward Compatibility (Optional)

  • My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:

(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)

Documentation (Optional)

  • In case of new functionality, my PR adds documentation in the following wiki page:

(Link the GitHub wiki you added)

Commits

  • My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Code Quality

  • My diff has been formatted using helix-style.xml
    (helix-style-intellij.xml if IntelliJ IDE is used)

@LZD-PratyushBhatt LZD-PratyushBhatt changed the title Lzd/ev update on is update Enable updating EV configs on IS updates Aug 14, 2025
@LZD-PratyushBhatt
Copy link
Author

@junkaixue @xyuanlu Can you review this? Thanks!

@xyuanlu
Copy link
Contributor

xyuanlu commented Sep 16, 2025

I am not sure about this "When a change is applied to the ideal_state of a resource, it’s expected that the EV reflect the changes. "
I think EV is aggregating CS. If no CS change, why we want to change EV?

@LZD-PratyushBhatt
Copy link
Author

Thanks for the review @xyuanlu
Regarding "I think EV is aggregating CS. If no CS change, why we want to change EV?"
Correct EV is constructed by aggregating CS, but lets say there is a change on IS simpleField that didnt trigger a CurrentStateChange event lets say, then in this case EV simplefields will not be matching with IS simplefields.

@xyuanlu
Copy link
Contributor

xyuanlu commented Sep 27, 2025

I think this is too expensive to add IS listener just for updating configs (map fields) in EV.
EV pipeline is pretty expensive. It computes EV for all resources by aggregating all CS. IS changes pretty frequently during rebalance, if we introduce an EV compute, it will cause a burden for the already overloaded Helix controller.
I recommend the user read the resource config from IS instead of EV.

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