Refactor code related to crash groups and linking mechanism #574

FelonEkonom · 2023-06-30T13:01:07Z

Related Jira ticekt: https://membraneframework.atlassian.net/browse/MC-191

mat-hek · 2023-07-19T10:04:33Z

lib/membrane/core/parent/child_life_controller.ex

@@ -653,45 +653,32 @@ defmodule Membrane.Core.Parent.ChildLifeController do
  end

  defp do_handle_child_death(child_name, :normal, state) do


this and the next clause became quite similar, I'd consider merging them

lib/membrane/core/parent/child_life_controller/crash_group_utils.ex

mat-hek · 2023-07-19T11:19:45Z

lib/membrane/core/parent/child_life_controller/crash_group_utils.ex

+
+  @spec handle_crash_group_member_death(Child.name(), CrashGroup.t(), any(), Parent.state()) ::
+          Parent.state()
+  def handle_crash_group_member_death(child_name, crash_group_data, reason, state)


this has so many clauses, can we make it a bit less verbose?

mat-hek · 2023-07-19T11:32:49Z

lib/membrane/core/element/pad_controller.ex

+  defp merge_pad_direction_data(pad_data, metadata, state) do
+    merge_pad_data(pad_data, &init_pad_direction_data(&1, metadata, state))
+  end
+
+  defp merge_pad_mode_data(pad_data, props, other_info, state) do
+    merge_pad_data(pad_data, &init_pad_mode_data(&1, props, other_info, state))
+  end


These add another level of complexity, which is one level too much IMO. If you don't like passing a function to merge_pad_data, I'd rather remove it and rename each init_pad_*_data function to merge_pad_*_data and explicitly call Map.merge there.

varsill · 2023-07-25T15:14:01Z

lib/membrane/core/element/pad_controller.ex

+    if pad_info.direction != :input,
+      do: raise("pad direction #{inspect(pad_info.direction)} is wrong")


is it even possible? If so, why don't we move it to the pattern match?

varsill · 2023-07-25T15:21:53Z

lib/membrane/core/element/pad_controller.ex

@@ -137,17 +130,18 @@ defmodule Membrane.Core.Element.PadController do
    end
  end

-  defp do_handle_link(:input, endpoint, other_endpoint, info, link_props, state) do
+  defp do_handle_link(:input, endpoint, other_endpoint, pad_info, link_props, state) do


perhaps we could "resolve" the names (endpoint -> input_endpoint etc.)?

the same applies for the do_handle_link(:output, ...)

varsill · 2023-07-26T07:18:13Z

lib/membrane/core/element/pad_controller.ex

      state =
        state.pads_data
        |> Map.values()
-        |> Enum.filter(&(&1.direction != data.direction and &1.flow_control == :auto))
-        |> Enum.reduce(state, fn other_data, state ->
-          PadModel.update_data!(state, other_data.ref, :associated_pads, &[data.ref | &1])
+        |> Enum.filter(&(&1.direction != pad_data.direction and &1.flow_control == :auto))
+        |> Enum.reduce(state, fn opposite_pad_data, state ->
+          PadModel.update_data!(
+            state,
+            opposite_pad_data.ref,
+            :associated_pads,
+            &[pad_data.ref | &1]
+          )


[NIT] I would opt for moving that part of code to some separate function, e.g. update_associated_pads or something like that.

varsill · 2023-07-26T07:28:19Z

lib/membrane/core/parent/child_life_controller.ex


    # adding crash group to state
    state =
-      if options.crash_group_mode != nil do
+      if options.crash_group_mode != nil or Map.has_key?(state.crash_groups, options.group) do


I am not sure I get it - shouldn't it be: if options.crash_group_mode != nil and (not Map.has_key?(state.crash_groups, options.group))
?

varsill · 2023-07-26T07:43:56Z

lib/membrane/core/parent/child_life_controller/crash_group_utils.ex

+    end
+  end
+
+  defp trigger_crash_group(crash_initiator, %CrashGroup{} = group, state) do


I am not the biggest fan of the trigger_crash_group name - I would opt at least for trigger_crash_group_crash, but it might sound funny :D
Perhaps you have some better alternative?

and when it comes to triggered? - I would rename it to crash_triggered? or something like that

mat-hek

Looks good 👍

lib/membrane/core/parent/child_life_controller/crash_group_utils.ex

test/membrane/core/element_test.exs

varsill · 2023-08-22T12:17:41Z

test/membrane/core/element_test.exs

@@ -130,10 +130,10 @@ defmodule Membrane.Core.ElementTest do
          %Endpoint{pad_spec: :output, pad_ref: :output, pad_props: %{options: []}, child: :this},
          output_other_endpoint,
          %{
-            other_info: other_info,
+            # other_info: other_info,


varsill · 2023-08-22T12:20:35Z

lib/membrane/core/parent/child_life_controller/crash_group_utils.ex

+          [Child.name()],
+          Parent.state()
+        ) :: Parent.state()
+  def extend_crash_group(group_name, _mode, children, state) do


I would remove mode argument here

Suggested change

def extend_crash_group(group_name, _mode, children, state) do

def extend_crash_group(group_name, children, state) do

FelonEkonom added the no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md label Jun 30, 2023

FelonEkonom force-pushed the refactor-crash-groups branch 5 times, most recently from a389db0 to 95b5d23 Compare July 11, 2023 15:36

Refactor CrashGroupUtils

233b0d2

FelonEkonom force-pushed the refactor-crash-groups branch from 95b5d23 to 233b0d2 Compare July 11, 2023 15:36

FelonEkonom added 2 commits July 11, 2023 17:48

Refactor CrashGroup

71a975c

Fix dialyzer

f148124

FelonEkonom marked this pull request as ready for review July 11, 2023 16:22

FelonEkonom requested a review from mat-hek as a code owner July 11, 2023 16:22

FelonEkonom added 4 commits July 12, 2023 10:51

Remove some calls to Bunch.Access

ccb91e9

Fix bugs in CrashGroupsUtils

259e4a0

Refactor wip

a9e0f10

Small refactor of pad controllers

c910d85

FelonEkonom force-pushed the refactor-crash-groups branch from 92e500a to ed3c26f Compare July 18, 2023 12:47

Merge remote-tracking branch 'origin/master' into refactor-crash-groups

809f17f

FelonEkonom force-pushed the refactor-crash-groups branch from ed3c26f to 809f17f Compare July 18, 2023 12:49

mat-hek requested changes Jul 19, 2023

View reviewed changes

FelonEkonom force-pushed the refactor-crash-groups branch from acef791 to cfa54e8 Compare July 19, 2023 14:58

Implement suggestions from CR

d3a16ea

FelonEkonom force-pushed the refactor-crash-groups branch from cf3ab1b to d3a16ea Compare July 20, 2023 12:02

FelonEkonom requested a review from mat-hek July 20, 2023 12:03

Merge remote-tracking branch 'origin/master' into refactor-crash-groups

435e9d4

FelonEkonom requested a review from varsill July 25, 2023 09:27

varsill requested changes Jul 26, 2023

View reviewed changes

FelonEkonom added 2 commits July 27, 2023 12:23

Implement suggestions from CR

9614b65

Implement suggestion from CR

332a724

FelonEkonom force-pushed the refactor-crash-groups branch from d7d57aa to 332a724 Compare July 28, 2023 13:13

mat-hek requested changes Aug 18, 2023

View reviewed changes

lib/membrane/core/parent/child_life_controller/crash_group_utils.ex Show resolved Hide resolved

Add comment abount removing items from crash group members

efa980b

FelonEkonom requested review from varsill and mat-hek August 21, 2023 14:13

Merge remote-tracking branch 'origin/master' into refactor-crash-groups

2d3a74c

mat-hek approved these changes Aug 21, 2023

View reviewed changes

varsill reviewed Aug 22, 2023

View reviewed changes

FelonEkonom added 2 commits August 22, 2023 16:34

Implement suggestions from CR

afbf53d

Merge remote-tracking branch 'origin/master' into refactor-crash-groups

511d50b

FelonEkonom requested a review from varsill August 22, 2023 14:35

varsill approved these changes Aug 22, 2023

View reviewed changes

FelonEkonom merged commit 68ab243 into master Aug 22, 2023

FelonEkonom deleted the refactor-crash-groups branch August 22, 2023 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor code related to crash groups and linking mechanism #574

Refactor code related to crash groups and linking mechanism #574

FelonEkonom commented Jun 30, 2023

mat-hek Jul 19, 2023

mat-hek Jul 19, 2023

mat-hek Jul 19, 2023

varsill Jul 25, 2023

varsill Jul 25, 2023

varsill Jul 26, 2023

varsill Jul 26, 2023

varsill Jul 26, 2023

varsill Jul 26, 2023

varsill Jul 26, 2023

mat-hek left a comment

varsill Aug 22, 2023

varsill Aug 22, 2023

		@@ -653,45 +653,32 @@ defmodule Membrane.Core.Parent.ChildLifeController do
		end

		defp do_handle_child_death(child_name, :normal, state) do

		if pad_info.direction != :input,
		do: raise("pad direction #{inspect(pad_info.direction)} is wrong")

	def extend_crash_group(group_name, _mode, children, state) do
	def extend_crash_group(group_name, children, state) do

Refactor code related to crash groups and linking mechanism #574

Refactor code related to crash groups and linking mechanism #574

Conversation

FelonEkonom commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mat-hek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment