apache / apache/buildstream

`_update_state()` may be gone, but it still lives on in our code structure

Open
#1,212 0 comments 0 reactions 0 assignees View on GitHub
refactoring
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.