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

Children groups #461

Merged
merged 82 commits into from
Dec 9, 2022
Merged

Children groups #461

merged 82 commits into from
Dec 9, 2022

Conversation

varsill
Copy link
Contributor

@varsill varsill commented Oct 24, 2022

This PR introduces children groups - a way to aggregate children so that all the children being part of a group can be referenced at once.

…d of 'links' and 'children' fiedls. Update the tests accordingly

Repair a bug with a trivial pipeline. Improve typespecs

Rename the LinkParser into the StructureParser

Add information about the children group in Membrane Logger's prints

Change the Membrane.TestingPipeline options specification so that to have the `structure` field

Improve functions naming in the StructureParser. Change the ParentSpec.spawn/2 into ParentSpec.spawn_child/2 to avois mistakes with Kernel.spawn

Add conditional link building

Change the return type of the ParenSpec.spawn_child/2 so that to unify the parent spec's structure field type

Fix a bug with a single element children list in link builder. Add typespecs for ignore_unless/2 and end_ignore/1

Rename to/3 into to_new/3

improve typespecs in parent spec

Fix a bug with nested conditional linking. Add tests checking if the nested conditional linking works properly

Add tests checking the behaviour of to_new/3

Revoke to/3 function

Refactor the `should_ignore` mechanism in parentspec

Add link_new/2 and tests checking it performance. Put the additional modules used in child_removal_test in separate files

Revert children groups implementation

Remove conditional linking

Rename ParentSpec into ChildrenSpec

Remove link, link_new, to, to_new, spawn_child functions. Add child/2, child/3, get_child/1 and get_child2

Add handling of ignore_duplicates option in child/3 and child/4. Revoke tests checking that functionality. Add is_bin_name?/1, is_element_name?/1 and is_child_name?/1 guards. Add default child options

Update ChildrenSpec moduledoc. Rename `to_bin_output` and `link_bin_input` into `bin_output` and `bin_input`, respectively. Rename `ignore_duplicates` option into `get_if_exists`.

Specify proper typespec for child/3

Improve moduledoc

Improve function docs in ChildrenSpec

Improve moduledoc in childrenspec

add test for element play

fixes for CR

Co-authored-by: Bartosz Błaszków <bartosz.blaszkow@swmansion.com>

handle_play -> handle_playing

play_request? -> playing_requested?

Play -> Playing

Apply suggestions from code review

Co-authored-by: Bartosz Błaszków <bartosz.blaszkow@swmansion.com>

refactor component path

Improved ChildrenSupervisor, ResourceGuard and Observability (#447)

* supervisors refactor

* resource guard

* add utility supervisor to context

* refactor element tests not to use State.new

* Update lib/membrane/resource_guard.ex

Co-authored-by: Bartosz Błaszków <bartosz.blaszkow@swmansion.com>

* run resource cleanup in a separate process

* refactor observability setup

* improve children supervisor API

* add docs and test for utility supervisor and resource guard

* add timeout to resource

* use a boring function name instead of go_brrr

* unlink cleanup task in resource guard before killing it

* Add list of available callbacks in each component (#449)

* Add an aggregated list of all callbacks that can be implemented by `Filter`, `Endpoint`, `Source` and `Sink` element behaviours.

Co-authored-by: Mateusz Front <mateusz.front@swmansion.com>

* add docs for Observability.setup

Co-authored-by: Bartosz Błaszków <bartosz.blaszkow@swmansion.com>
Co-authored-by: Łukasz Kita <lukasz.kita0@gmail.com>

Make `dont_spawn_if_already_exists` one of keywords for options in child_spec. Update the tests with new syntax for spawning children
… concerning the children spawning. Repair a bug with ChildLifeController.handle_child_death typespec. Repair typespec of Pipeline.State struct
…d/3` function set the structure builder status to :done
…dd an ability to remove all the children from given children group
@varsill varsill marked this pull request as ready for review October 24, 2022 15:36
@varsill varsill requested a review from mat-hek as a code owner October 24, 2022 15:36
varsill and others added 19 commits October 26, 2022 11:41
…ormation to error print in the structure parser
…truct. Add an ability to remove all the children from given children group"

This reverts commit 1a9a191.
* Validate link in ChildrenSpec

* Add tests for links validation
* Match caps in bin's pads

* Caps specs as Elixir patterns
* Rename def_*_pad option from :caps to :accepted_format
* Rename :caps to :stream_format
@varsill varsill requested a review from mat-hek December 1, 2022 11:36
Comment on lines 344 to 349
+ child(:filter, SomeFilter),
+ get_child(:source) |> child(:another_filter, SomeFilter) |> get_child(:filter) |> child(:sink, SomeSink)
]
+ spec = {structure, children_group_id: :first_group, crash_group_mode: :temporary, node: another_node}
+ spec = {structure, group: :first_group, crash_group_mode: :temporary, node: another_node}
- {{:ok, spec: spec}, %{}}
+ {[spec: spec], %{}}
Copy link
Member

Choose a reason for hiding this comment

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

this won't be in 0.11 ;)

Copy link
Contributor Author

@varsill varsill Dec 5, 2022

Choose a reason for hiding this comment

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

I have removed that a few commits before, I don't know how has it appeared there and why was it still visible to you :D

node: node() | nil,
log_metadata: Keyword.t()
}
@opaque parsed_children_spec_options_t :: %{
Copy link
Member

Choose a reason for hiding this comment

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

Why not just @type?

Copy link
Contributor Author

@varsill varsill Dec 5, 2022

Choose a reason for hiding this comment

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

Well, this type is used externally only by StartupUtil.scheck_if_children_names_and_children_groups_ids_are_unique - and that function does not manipulate the internals of the type. In fact, StartupUtil.scheck_if_children_names_and_children_groups_ids_are_unique needs to use of that type since there is a need to emphasize, that the input argument is in the canonical form - perhaps we should add a children_spec_canonical_form_t type than, and make parsed_children_spec_options_t private? WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Good idea ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 222 to 225
# child name created with child(...)
{:__membrane_just_child_name__, child_name} -> Membrane.Child.ref(child_name, group: group)
# child name created with get_child(...), bin_input() and bin_output()
ref -> ref
Copy link
Member

Choose a reason for hiding this comment

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

If all child names were wrapped, we could match them like this:

Suggested change
# child name created with child(...)
{:__membrane_just_child_name__, child_name} -> Membrane.Child.ref(child_name, group: group)
# child name created with get_child(...), bin_input() and bin_output()
ref -> ref
# child name created with child(...)
{:child_name, child_name} -> Membrane.Child.ref(child_name, group: group)
# child name created with get_child(...), bin_input() and bin_output()
{:child_ref, ref} -> ref

It looks cleaner IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


@type name_t :: Element.name_t() | Bin.name_t()
@type group_t() :: any()
@type ref_t :: {:__membrane_children_group_member__, group_t(), name_t()} | name_t()
Copy link
Member

Choose a reason for hiding this comment

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

I've realized that this is going to be printed out in each log... Maybe we could have

Suggested change
@type ref_t :: {:__membrane_children_group_member__, group_t(), name_t()} | name_t()
@type ref_t :: {Membrane.Child, group_t(), name_t()} | name_t()

like {Membrane.Pad, name, id}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, done

[{spec, options}]
end

defp equip_spec_with_children_refs(specs, group) do
Copy link
Member

Choose a reason for hiding this comment

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

actually these are structure builders

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, done

[{spec, options}]
end

defp equip_spec_with_children_refs(specs, group) do
specs =
Enum.map(specs, fn spec ->
Copy link
Member

Choose a reason for hiding this comment

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

maybe update both children and links in one pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, sure ;) Done

@varsill varsill requested a review from mat-hek December 5, 2022 15:45
* Get rid of using structure term in context of children specs. Update the tests accordingly. Removed unused support element pipeline
@varsill varsill merged commit f9f9381 into master Dec 9, 2022
@varsill varsill deleted the children_groups branch December 9, 2022 08:25
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.

3 participants