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

async stream + timeout #1593

Merged
merged 6 commits into from
Jun 20, 2020
Merged

Conversation

InoMurko
Copy link
Contributor

📋 Add associated issues, tickets, docs URL here.

Overview

Describe what your Pull Request is about in a few sentences.

Changes

Describe your changes and implementation choices. More details make PRs easier to review.

  • Change 1
  • Change 2
  • ...

Testing

Describe how to test your new feature/bug fix and if possible, a step by step guide on how to demo this.

put_timestamp_and_sft(exit_event, state.min_exit_period_seconds, state.child_block_interval)
end,
timeout: 50_000,
on_timeout: :kill_task,
Copy link
Contributor

Choose a reason for hiding this comment

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

are we sure we don't want to exit the process?

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, and I refresh the page and is changed to exit 😅

Copy link
Contributor Author

@InoMurko InoMurko Jun 18, 2020

Choose a reason for hiding this comment

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

no :) you were 32seconds too quick

Copy link
Contributor

@boolafish boolafish left a comment

Choose a reason for hiding this comment

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

🔥

end,
timeout: 50_000,
on_timeout: :exit,
max_concurrency: System.schedulers_online() * 2
Copy link
Contributor

Choose a reason for hiding this comment

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

this would not be larger then our rate limit to infura right (is there a rate limit)? (btw, I actually have no idea what both number are 😅)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the beam installs a scheduler per CPU core. so if you have 8 cores times 2, you'd do at most 16 exits at the same time.

@@ -175,7 +175,7 @@ defmodule OMG.Watcher.ExitProcessor do
but under unchanged conditions, it should have unchanged behavior from POV of an outside caller.
"""
def check_validity() do
GenServer.call(__MODULE__, :check_validity)
GenServer.call(__MODULE__, :check_validity, 60_000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add this value to module attribute and reuse it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah!

Copy link
Contributor

@unnawut unnawut left a comment

Choose a reason for hiding this comment

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

LGTM! Also a smart plan to merge to master so we can observe the async stream behaviour for a bit longer

fn exit_event ->
put_timestamp_and_sft(exit_event, state.min_exit_period_seconds, state.child_block_interval)
end,
timeout: 50_000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh this Task.async_stream's :timeout is very handy 👏

Enum.map(
events,
events
|> Task.async_stream(
Copy link
Contributor

Choose a reason for hiding this comment

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

ExPlasma.InFlightExit.txbytes_to_id(txbytes) is already a local computation though, no longer making RPC calls.

Maybe at least we can remove the custom timeout and so defaults back to 5000ms which is plenty enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh geez. I didn't even check. Now I remember you did this!

@InoMurko InoMurko merged commit 3062461 into master Jun 20, 2020
@InoMurko InoMurko deleted the inomurko/watcher_parallel_exit_processing branch June 20, 2020 14:11
@unnawut unnawut added the bug Something isn't working label Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants