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

Add naming to flow-generated processes #67

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MarcVanOevelen
Copy link

Implements stage naming as discussed in 'Naming stages #48'
Optional automatic generated unique names, effective when the flow is explicitly named
Automatic stage names can be overruled using :name option.
See the module doc for details.

Optional automatic generated unique names
Optionally overruled using :name
lib/flow/coordinator.ex Outdated Show resolved Hide resolved
lib/flow/coordinator.ex Outdated Show resolved Hide resolved
lib/flow/materialize.ex Outdated Show resolved Hide resolved
lib/flow.ex Outdated Show resolved Hide resolved
lib/flow.ex Outdated Show resolved Hide resolved
@josevalim
Copy link
Member

Thanks @MarcVanOevelen, i have added some comments and also please be sure to run the code formatter, otherwise the build won't pass. Thanks!

@josevalim
Copy link
Member

josevalim commented Sep 11, 2018 via email

apply proper code formatting
@MarcVanOevelen
Copy link
Author

@josevalim , I also wanted to include a test for the exception case when specifying a partition name on an unnamed flow but couldn't find a way to suppress the GenServer error log.
My test looks as follows :

test "error when naming stage for unnamed flow" do
Process.flag(:trap_exit, true)
message = "a partition is named ':output', this requires the flow to be named"
{:error, reason} = Flow.from_enumerable([1,2,3])
|> Flow.map(&(&1 + 1))
|> Flow.partition(stages: 3, name: :output)
|> Flow.reduce(fn -> 0 end, &(&1 + &2))
|> Flow.start_link()
assert {%ArgumentError{message: ^message}, _} = reason
end

The test passes but the GenServer termination error log is printed :

2:28:21.176 [error] GenServer #PID<0.158.0> terminating
** (ArgumentError) a partition is named ':output', this requires the flow to be named
(flow) lib/flow/materialize.ex:71: Flow.Materialize.start_stages/7
(flow) lib/flow/materialize.ex:22: Flow.Materialize.materialize/5
(flow) lib/flow/coordinator.ex:28: Flow.Coordinator.init/1
(stdlib) gen_server.erl:365: :gen_server.init_it/2
(stdlib) gen_server.erl:333: :gen_server.init_it/6
(stdlib) proc_lib.erl:247: :proc_lib.init_p_do_apply/3
Last message: {:EXIT, #PID<0.157.0>, {%ArgumentError{message: "a partition is named ':output', this requires the flow to be named"}, [{Flow.Materialize, :start_stages, 7, [file: 'lib/flow/materialize.ex', line: 71]}, ...

Any idea how to fix this ?

@josevalim
Copy link
Member

josevalim commented Sep 11, 2018 via email

@MarcVanOevelen
Copy link
Author

Yes, already tried that but it makes no difference :-(

Marc Van Oevelen and others added 3 commits November 1, 2018 14:20
Optional automatic generated unique names
Optionally overruled using :name
apply proper code formatting
@rafaelwkerr
Copy link

@MarcVanOevelen and @josevalim this PR will be merged?

@josevalim
Copy link
Member

Sorry for dropping this one. First we need to rebase and we should probably focus this PR on the name changes.

@josevalim
Copy link
Member

Hi @MarcVanOevelen. Sorry for the delay, do you still have interest in getting this in? If so, can you please rebase it? Thanks.

@a8t
Copy link

a8t commented Jun 6, 2021

I rebased this out of curiosity as a way to dig into the internals, but I did something wrong in Git and the commits are attributed to me... if that's OK, I can re-open the PR. Though I'm sure if it's that wise, considering I don't have the technical ability to really own the PR.

@josevalim lmk?

this is what a PR from my rebased branch to current master woudl look like: https://github.com/a8t/flow/pull/1/files

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.

4 participants