Skip to content

add support for naming tasks, so that they can't be scheduled twice #1

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

Closed
wants to merge 1 commit into from

Conversation

taybin
Copy link
Contributor

@taybin taybin commented Jan 13, 2013

-- also added support for getting status and canceling via names.

@taybin
Copy link
Contributor Author

taybin commented Jan 13, 2013

A couple comments.

The main reason for this was to solve the problem of which node amongst many would schedule the long running tasks on launch.

While it might be a faster lookup to put the Name -> Pid mapping in a gb_tree or dict, it's a hassle to delete the name if the task is later deleted via pid.

With the current implementation, if you don't use names, you don't pay for them.

@jeraymond
Copy link
Owner

This is a useful feature, thanks for the submission. I've a few suggestions/fixes before merging this in:

  • use the atom undefined instead of an empty list for unnamed tasks
    • this would exclude using undefined as a registered name, but would make the task definition tuple consistent
  • update the task() type definition to include the name as part of the the definition (line 73)
  • make sure all tests pass and the static analysis succeeds
    • the common test is failing due to the change in the task definition signature (test/leader_cron_SUITE.erl)
    • dialzyer is failing because of a bug introduced on line 361
  • fix bug introduced on line 361 (should be a call to leader_cron_task:start_link/2 not leader_cron:start_link/2)
  • add edocs for new schedule_task/3 function
  • minor style tweaks
    • for all case statements put the case body on its own line
    • for the commit message, start the line liner description with a capital letter

I've updated the README.md to include instructions for running all tests and the static analysis.

@taybin
Copy link
Contributor Author

taybin commented Jan 13, 2013

This is funny because I used undefined at first, but thought that arbitrarily not allowing undefined was a shame.

I'll make the changes. Glad you like the idea though.

Mostly, so that they can't be scheduled twice
Also added support for getting status and canceling via names.
@taybin
Copy link
Contributor Author

taybin commented Jan 13, 2013

New commit with requested updates.

The CT tests seemed to pass before, so there might be something wrong with my setup. I'm running Erlang R14B04.

There were a couple other errors in the dialzyer output regarding run_tasks() never returning when using a sleeper task. I think those were already there though. If not, I could use a hint on how I could have introduced that.

@jeraymond
Copy link
Owner

Yes the run_tasks warnings are expected. Dialyzer doesn't like that for some inputs run_tasks never returns (periodic tasks) for some it does return (oneshot). Normally that type of behaviour would indicate a programming error. If you run dialyzer via the Makefile (make dialyzer) it'll compare these expected warnings to the dialyzer_reference file and ignore them.

I pulled down your change locally and fixed the common test (see test/leader_cron_SUITE.erl). The problem was with the task tuple now also returning the name (similar to the change you made to the eunit tests).

I also cleaned up a few other minor things like ensuring the line lengths were not > 80 characters. I've pushed our combined changes to master.

Thanks for the submission!

@jeraymond jeraymond closed this Jan 14, 2013
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.

2 participants