`_update_state()` may be gone, but it still lives on in our code structure
- Dominant language
- Python
- Stars
- 140
- Forks
- 45
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 6
Description
[See original issue on GitLab](https://gitlab.com/BuildStream/buildstream/-/issues/1212)
In GitLab by [[Gitlab user @tlater]](https://gitlab.com/tlater) on Nov 20, 2019, 13:18
## Background
[//]: # (Provide a background or the root/source that justifies this task or action.)
We [recently removed
`Element._update_state`](https://gitlab.com/BuildStream/buildstream/merge_requests/1660),
a function which had to be used whenever we suspected that the state
of an element in the main thread may have changed, so that it could go
and re-compute the state. We did so because it was slow, and far more
complex than it needed to be.
Our eventual goal is to move to a push-based system (i.e., when we do
something that causes state to change, notify the element of the
change, rather than asking it to figure out if state has changed), but
we are not quite there. `_update_sate()` has been split into smaller
functions that update specific states when prompted - this means we go
through slightly less code, and we are closer to a push-based system
(since at least we update *specific* states) but it's not yet ideal.
There are also some remaining points in #902 that need to be addressed
(although that issue has become somewhat cluttered).
I'd like to track what still needs to be done in this issue, but
first, as a summary, here are the locations that still update an
elements' state:
1. `Element._initialize_state`
2. Reverse dependency updates in `__update_ready_for_runtime` and `__update_strict_cache_key_of_rdeps`
3. `Element._set_required`
4. `Element.__schedule_assembly_when_necessary`
5. `Element._tracking_done`
6. `Element._pull_done`
7. `Element._assemble_done`
8. `Element._fetch_done`
## Task description
[//]: # (Short summary of the action to be executed)
In summary, I believe the following still needs to happen:
* [ ] Move state handling into `Element._initialize_state`
Since it actually is responsible for learning the current state of
elements before we have any information, most of the current state
updating should probably move into this method, and only be
performed once.
* [ ] `Element._*_done` should receive the changed state
Each of these methods probably do a lot of superfluous work; They
should be passed the state to set from child processes, instead of
re-computing the world.
* [ ] `Element._set_required` and
`Element.__schedule_assembly_when_necessary` are almost
identical in functionality
One of these methods should disappear
* [ ] Interfaces between different `_update_state` component methods should also become push-based
## Acceptance Criteria
[//]: # (Acceptance criteria should follow the S.M.A.R.T. principle https://en.wikipedia.org/wiki/SMART_criteria )
BuildStream's state handling is no longer done through a magic
`Element._update_state()` method that figures out what's going on, but
is instead done in a push-oriented way where updates to the state are
communicated to the element.
----
[//]: # (To review information about possible relevant labels for this issue please view the list of labels: https://gitlab.com/BuildStream/buildstream/labels)
Contributor guide
Research direction
Start with Element._initialize_state and the listed state-update methods, including the _*_done callbacks, _set_required, and __schedule_assembly_when_necessary; review the remaining points in #902 for context. Done means state handling no longer relies on a magic Element._update_state() method and updates are communicated in a push-oriented way.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- build-system
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100