Add an ability to schedule jobs to be executed in the future.

eugenijm commented

2019-04-10 00:30:08 +00:00

Member

Closes #2

This can be used for scheduled activities pleroma/pleroma#4375

Closes #2 This can be used for scheduled activities https://git.pleroma.social/pleroma/pleroma/pulls/4375#note_23055

eugenijm commented

2019-04-10 20:03:28 +00:00

Author

Member

Going to rework this a bit to properly handle entries with the same timestamps.
Edit: Done

Going to rework this a bit to properly handle entries with the same timestamps. Edit: Done

eugenijm commented

2019-04-12 14:23:28 +00:00

Author

Member

The idea is that persistence can be added if needed by creating and plugging in a different store (e.g. mnesia-based).

minibikini commented

2019-04-15 06:30:35 +00:00

Member

❤️

:heart:

minibikini commented

2019-04-15 06:48:53 +00:00

Member

Markdown lists style is not consistent: some use - and some *. I don't care which one we use, but maybe * is better because it's used by Pleroma.

Please, make sure all lists have the same symbol.

Markdown lists style is not consistent: some use `-` and some `*`. I don't care which one we use, but maybe `*` is better because it's used by Pleroma. Please, make sure all lists have the same symbol.

minibikini commented

2019-04-15 07:00:31 +00:00

Member

One # per line is enough ;)

One `#` per line is enough ;)

minibikini commented

2019-04-15 07:13:30 +00:00

Member

Here and also line 17: need to add a default value [] to Application.get_env/3, like this:

|> Application.get_env(:scheduler, [])

Otherwise, if there is no configuration, it would fail with FunctionClauseError.

Here and also line 17: need to add a default value `[]` to `Application.get_env/3`, like this: ```elixir |> Application.get_env(:scheduler, []) ``` Otherwise, if there is no configuration, it would fail with `FunctionClauseError`.

minibikini commented

2019-04-15 07:16:01 +00:00

Member

Please, add a default value to Application.get_env/3.

Please, add a default value to `Application.get_env/3`.

minibikini commented

2019-04-15 07:20:52 +00:00

Member

Can you please elaborate on why we need it here?

minibikini commented

2019-04-15 07:22:48 +00:00

Member

I think it should be false by default. We have another pleroma related project which uses the queue and it doesn't need scheduled jobs.

I think it should be `false` by default. We have another pleroma related project which uses the queue and it doesn't need scheduled jobs.

eugenijm commented

2019-04-15 07:34:50 +00:00

Author

Member

Mainly for tests. What do you think about moving it to config/test.exs? Feels a bit better than adding Application.put_env in test files to enable it since it's going to be disabled by default

Mainly for tests. What do you think about moving it to `config/test.exs`? Feels a bit better than adding `Application.put_env` in test files to enable it since it's going to be disabled by default

minibikini commented

2019-04-15 07:43:51 +00:00

Member

PleromaJobQueue.Scheduler.Store behavior states that init/1 may also return {:error, error :: any()}.

I suggest to wrap it into with statement, so a potential store error won't cause a MatchError. Something like this:

def init(_) do
  with true <- PleromaJobQueue.enabled?() and enabled?(),
        {:ok, state} <- @store.init([]) do
    schedule_next(:poll)
    {:ok, state}
  else
    _ -> :ignore
  end
end

`PleromaJobQueue.Scheduler.Store` behavior states that `init/1` may also return `{:error, error :: any()}`. I suggest to wrap it into `with` statement, so a potential store error won't cause a `MatchError`. Something like this: ```elixir def init(_) do with true <- PleromaJobQueue.enabled?() and enabled?(), {:ok, state} <- @store.init([]) do schedule_next(:poll) {:ok, state} else _ -> :ignore end end ```

minibikini commented

2019-04-15 07:48:31 +00:00

Member

I see. When you add default values to all Application.get_env calls it will work without config (I've just tried and it worked on my machine).

I see. When you add default values to all `Application.get_env` calls it will work without config (I've just tried and it worked on my machine).

eugenijm commented

2019-04-15 07:56:19 +00:00

Author

Member

Probably you tried it with enabled: true. With enabled set to false by default, the scheduler doesn't start in tests, so they don't pass. Confirmed it on my machine :)

Probably you tried it with `enabled: true`. With `enabled` set to `false` by default, the scheduler doesn't start in tests, so they don't pass. Confirmed it on my machine :)

minibikini commented

2019-04-15 08:13:29 +00:00

Member

Oh right. :face_palm: Yeah, in this case we need config :pleroma_job_queue, :scheduler, enabled: true let's put it in config/test.exs.

Oh right. :face\_palm: Yeah, in this case we need `config :pleroma_job_queue, :scheduler, enabled: true` let's put it in `config/test.exs`.

minibikini commented

2019-04-15 08:28:28 +00:00

Member

I just updated the CI config in master, it should fix the pipeline.

minibikini commented

2019-04-15 08:37:40 +00:00

Member

Hmm, {:ok, state} = @store.init([]) should be {:ok, state} <- @store.init([]) otherwise it would still fail with MatchError.

Also, please don't force push it's hard to track changes.

Hmm, `{:ok, state} = @store.init([])` should be `{:ok, state} <- @store.init([])` otherwise it would still fail with `MatchError`. Also, please don't force push it's hard to track changes.

eugenijm commented

2019-04-15 08:41:46 +00:00

Author

Member

Nice catch, done. Re: commit history, sure, noted.

eugenijm commented

2019-04-15 10:04:26 +00:00

Author

Member

Thanks. I also updated the config so that it doesn't fail in a non-test env

eugenijm commented

2019-04-15 10:05:51 +00:00

Author

Member

Resolved this. The only place where - is used is the changelog. This is to be consistent with the Pleroma's changelog.

Resolved this. The only place where `-` is used is the changelog. This is to be consistent with the Pleroma's changelog.

Pull request closed

Please reopen this pull request to perform a merge.

Rows
Columns

Add an ability to schedule jobs to be executed in the future. #6

Pull request closed