Storage adapter implementation: Implemented Quantum.Storage.Adapter behavior via PersistentETS. #318

pyatkov · 2018-03-05T12:52:14Z

Provides an implementation of the Quantum.Storage.Adapter behaviour via Persistent ETS library.
The specialty is that the behaviour is implemented as a GenServer and requires explicit starting as a worker process before the scheduler app.
The whole need for GenServer-ish behaviour appeared due to default :protected access to the ETS table (which implies only the process that created the table can have write access to it). GenServer-ish behaviour allows us to have a dedicated process to access the ETS table thus resolving this issue.
We could have considered the table to be :public, but things inclined the GenServer-ish approach.

We also don't have ane tests targeting specifically this implementation module. Still, we could probably test the functionality by configuring this storage adapter via the :storage option in the application configuration.

maennchen · 2018-03-05T13:26:00Z

@pyatkov Can you do me a favor and rebase again on the branch? I had to merge in some changes from the master.

maennchen · 2018-03-05T13:31:03Z

mix.exs

@@ -80,7 +80,8 @@ defmodule Quantum.Mixfile do
      {:excoveralls, "~> 0.5", only: [:dev, :test], runtime: false},
      {:inch_ex, "~> 0.5", only: [:dev, :docs], runtime: false},
      {:dialyxir, "~> 0.5", only: [:dev, :test], runtime: false},
-      {:credo, "~> 0.7", only: [:dev, :test], runtime: false}
+      {:credo, "~> 0.7", only: [:dev, :test], runtime: false},
+      {:persistent_ets, "~> 0.1.0"},


Make the dependency optional. Not everyone will need the storage.

Just an observation: here and in many many places, diligent coders have to specify runtime: false as they don't need the application to start (sometimes there is no application defined in the package). Just wondering if it was really a good move to make the dependencies start as an app by default? Kind of a move towards the newcomers, but in about a week or so, they become aware of the apps and become concerned about the runtime: false options as all the others do.
For me, the old good explicit aplication start looked a more neat and manture option.

I'll do the rebase.

@pyatkov I'd rather not move away from the standard in Elixir. In my professional opinion, it will be even more complicated if every other place does this differently.

@maennchen
As I said, it was Just an observation., so ...
BTW, this is my personal account whose repo I use for developing the PR.

maennchen · 2018-03-05T13:32:46Z

lib/quantum/storage/persistent_ets.ex

@@ -0,0 +1,195 @@
+defmodule Quantum.Storage.PersistentEts do


Wrap the defmodule in if Code.ensure_compiled?(PersistentEts) do .... end

maennchen · 2018-03-05T13:36:33Z

@pyatkov I think we can implement a global test suite for every adapter. I'll have a look at that after the merge.

maennchen · 2018-03-05T13:46:18Z

test/quantum/execution_broadcaster_test.exs

+        # Maybe one second elapsed in the test?
+        assert_receive {:received, {:execute, ^job}}, 1000
+
+        # Goes back to normal pace


This is a bug of my test. I'm going to fix it in the storage branch.

… storage # Conflicts: # lib/quantum/job_broadcaster.ex # test/quantum/execution_broadcaster_test.exs # test/quantum/job_broadcaster_test.exs

… not Application to be started). Made Quantum.Storage.PersistentEts module definition optional depending on whether dependent PersistentEts module is compiled and loaded.

maennchen · 2018-03-07T12:01:44Z

lib/quantum/job_broadcaster.ex

@@ -34,8 +34,10 @@ defmodule Quantum.JobBroadcaster do

  @doc false


I think your merge was not clean. What changes did you make here?

Yeah I messed up a couple of times doing the rebase stuff...
I think I fixed that in the latest commit.
Still TravisCI still complains about the formatting issues.
Though when I try to do mix format a while bunch of changes pops up.
I believe I should only commit format changes to the files Travis is complaining about (there will be three of them).
I'll try do to that shortly.

maennchen · 2018-03-07T12:01:53Z

lib/quantum/job_broadcaster.ex

@@ -153,6 +159,8 @@ defmodule Quantum.JobBroadcaster do

    :ok = storage.purge(scheduler)

+    :ok = storage.purge(scheduler)


Why purge twice?

…ation though (otherwise it does not work). Just wasn't aware of this.

dapdizzy

Made a note about code changes in job_broadcaster.ex.

dapdizzy · 2018-03-07T12:17:19Z

lib/quantum/job_broadcaster.ex

-      jobs: Enum.into(jobs, %{}, fn %{name: name} = job -> {name, job} end),
-      buffer: for(%{state: :active} = job <- jobs, do: {:add, job}),
+      jobs: Enum.into(effective_jobs, %{}, fn %{name: name} = job -> {name, job} end),
+      buffer: for(%{state: :active} = job <- effective_jobs, do: {:add, job}),
      storage: storage,
      scheduler: scheduler


Just realized that the existing code was rebinding the jobs variable, so...effective_jobs variable is basically not needed.
I could probably stick with the jobs variable.
I mean this part could be reverted (what existed in the master repo storage branch is just fine).

It's good like this 👍

maennchen · 2018-03-07T12:53:07Z

@dapdizzy I'm going to merge into storage. Be aware that I want to make sure that everything is stable before it will be released. So it may take a few days until this will be live.

maennchen · 2018-03-07T12:53:16Z

Thank you very much!

maennchen · 2018-03-07T13:02:32Z

Merged manually.

dapdizzy · 2018-03-07T13:12:31Z

Thank you!!! Couldn't imagine it would be that great! From the naive test perspective, I schedule a job, then press 'Ctrl+C' twice, then run 'iex -S mix' again and the jobs are picked from the storage just fine. I'm aware this needs through testing, just saying. Note that ':persistent_ets' dep should be included into the client app using ':quantum' explicitly, otherwise it will not be obtained (I believe you are aware if this, but I faced it for the first time though). Also 'Quantum.Storage.PersistentEts' should be started as a worker (without args) explicitly before the Scheduler process in order to work. It was a great experience working on this little piece of software. I really appreciate it. The funny thing is that I'm working full-time as a Dynamics AX developer, though I don't see myself doing this long term. I highly tend to work in Elixir, so this was a great opportunity for me to add my two cents to the lib. Would love to get my hands on developing new PRs soon! Ср, 7 марта 2018 г. в 15:53, Jonatan Männchen <notifications@github.com>:

…

Thank you very much! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/c-rack/quantum-elixir/pull/318#issuecomment-371129073>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD4VeH-3xwp2wek-EYFVO0qeK7HSVb_Nks5tb9hDgaJpZM4ScPDk> .

maennchen · 2018-03-07T13:50:49Z

Couldn't imagine it would be that great!

I hope it is :)

I'm aware this needs through testing, just saying.

I'm going to write a general test suite for all connectors.

Note that ':persistent_ets' dep should be included into the client app using ':quantum' explicitly, otherwise it will not be obtained

We're going to mention that in the documentation.

Would love to get my hands on developing new PRs soon!

There's a few idea tickets if you're searching for some inspiration: https://github.com/c-rack/quantum-elixir/issues?q=is%3Aissue+is%3Aopen+label%3Aidea

As soon as we have storage in stable, it would be cool if we could tackle #314. I'd really appreciate your help on it.

c-rack · 2018-03-07T19:46:55Z

@pyatkov @dapdizzy thanks a lot for your contributions!
Are you the same person using two github accounts? 😉

dapdizzy · 2018-03-08T10:41:09Z

Yeah! DapDizzy is my personal one and Pyatkov is the one my employer asked me to create (just for the ease of management as it corresponds to my surname and Corp account). It was a pleasure adding bits to this library! Ср, 7 марта 2018 г. в 22:47, Constantin Rack <notifications@github.com>:

…

@pyatkov <https://github.com/pyatkov> @dapdizzy <https://github.com/dapdizzy> thanks a lot for your contributions! Are you the same person using two github accounts? 😉 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/c-rack/quantum-elixir/pull/318#issuecomment-371261310>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD4VeKlbAEI-xqLs67m9IOd7NmCA_gGWks5tcDkygaJpZM4ScPDk> .

mertkahyaoglu · 2018-09-02T19:13:09Z

Thank you very much for this feature. I really needed to persist runtime jobs. How can I add this to my project? Is there any documentation?

maennchen · 2018-09-02T20:21:41Z

Sure, check this documentation page: https://hexdocs.pm/quantum/Quantum.Storage.Adapter.html#content

mertkahyaoglu · 2018-09-03T20:09:39Z

@maennchen I'm trying to install quantum_storage_ets but it says;

(Mix) No package with name quantum_storage_ets (from: mix.exs) in registry.

How am I suppose to use this adapter?

dapdizzy · 2018-09-04T05:11:00Z

This particular detail seems to be missing (or located somewhere else) in the documentation.

In order to make it work, you need to do the following:
':persistent_ets' dep should be included into the client app (your app)
using ':quantum' explicitly, otherwise it will not be obtained (I believe
you are aware if this, but I faced it for the first time though).
Also 'Quantum.Storage.PersistentEts' should be started as a worker (without
args) explicitly before the Scheduler process in order to work.

I think these two things should make it work.

@mertkahyaoglu

So, the dependency name is 'persistent_ets' and a worker for a module 'Quantum.Storage.PersistentEts' without arguments (i.e. '[]') is required to be started prior to the scheduler process.

maennchen · 2018-09-04T05:36:18Z

@mertkahyaoglu The dependency is not yet on hex. I forgot to publish it there. I‘ll do that today.

Until then you can install it via git.

maennchen · 2018-09-04T05:38:00Z

@dapdizzy Most things about you comment above is no longer correct since it was extracted into a separate dependency.

mertkahyaoglu · 2018-09-04T06:12:33Z

Most things about you comment above is no longer correct since it was extracted into a separate dependency.

@maennchen After installing the dependency, I need to add the worker to my supervisor though right? Then I can add my jobs to storage.

Sorry if I'm asking obvious questions. I'm really new to Elixir 😅 Thanks so much again for helping!

dapdizzy · 2018-09-04T06:15:03Z

Oh, sorry about that. Just thought the stone was on my side. вт, 4 сент. 2018 г. в 9:12, Mert Kahyaoğlu <notifications@github.com>:

…

Most things about you comment above is no longer correct since it was extracted into a separate dependency. After installing the dependency, I need to add the worker to my supervisor though right? Then I can add my jobs to storage. Sorry if I'm asking obvious questions. I'm really new to Elixir 😅 Thanks so much again for helping! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#318 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD4VeDFg9gTOmDCnl9seBxVb2uvxXHVmks5uXhnRgaJpZM4ScPDk> .

mertkahyaoglu · 2018-09-04T17:53:26Z

I managed to store my jobs. Now I need to load them when I restarted my server. I suppose they are not being started automatically. Could you guys help me one last time please?

maennchen · 2018-09-04T20:16:10Z

@mertkahyaoglu They should actually ve restarted automatically. Could you enable debug logging and open a new issue with that log so I can track it down?

maennchen · 2018-09-04T20:17:45Z

@dapdizzy No worries! I just thought that I have to correct that with the extracted project.

WIP: Solution: Storage Adapter

3ce0364

pyatkov changed the title ~~Storage adapter implementation: Implemented Quantum.Storage.Adapter bahavior via PersistentETS.~~ Storage adapter implementation: Implemented Quantum.Storage.Adapter behavior via PersistentETS. Mar 5, 2018

pyatkov mentioned this pull request Mar 5, 2018

Problem: No persistent Quantum State #240

Closed

maennchen self-requested a review March 5, 2018 13:24

maennchen self-assigned this Mar 5, 2018

maennchen added the enhancement label Mar 5, 2018

maennchen requested changes Mar 5, 2018

View reviewed changes

maennchen reviewed Mar 5, 2018

View reviewed changes

Initial implementation of Quantum.Storage.Adapter via PersistentEts.

e929bd3

dapdizzy force-pushed the storage branch from a6bc03a to e929bd3 Compare March 7, 2018 10:34

Merge branch 'storage' of git://github.com/c-rack/quantum-elixir into…

cfdbd2e

… storage # Conflicts: # lib/quantum/job_broadcaster.ex # test/quantum/execution_broadcaster_test.exs # test/quantum/job_broadcaster_test.exs

maennchen changed the base branch from storage to master March 7, 2018 11:21

maennchen changed the base branch from master to storage March 7, 2018 11:21

Made :persistent_ets dependency optional and non-runtime (as there is…

e8e4cb8

… not Application to be started). Made Quantum.Storage.PersistentEts module definition optional depending on whether dependent PersistentEts module is compiled and loaded.

maennchen requested changes Mar 7, 2018

View reviewed changes

pyatkov added 3 commits March 7, 2018 15:05

Fixed merging issues in job_broadcaster.ex

853e6db

Fixing formatting of the 3 files TravisCI is complaining about.

4df5067

Found out that the persistent_ets dependency should start an Applic…

d31e291

…ation though (otherwise it does not work). Just wasn't aware of this.

dapdizzy reviewed Mar 7, 2018

View reviewed changes

maennchen approved these changes Mar 7, 2018

View reviewed changes

maennchen closed this Mar 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage adapter implementation: Implemented Quantum.Storage.Adapter behavior via PersistentETS. #318

Storage adapter implementation: Implemented Quantum.Storage.Adapter behavior via PersistentETS. #318

pyatkov commented Mar 5, 2018

maennchen commented Mar 5, 2018

maennchen Mar 5, 2018

pyatkov Mar 6, 2018

maennchen Mar 6, 2018

dapdizzy Mar 7, 2018 •

edited

Loading

maennchen Mar 5, 2018

maennchen commented Mar 5, 2018

maennchen Mar 5, 2018

maennchen Mar 7, 2018

dapdizzy Mar 7, 2018

maennchen Mar 7, 2018

dapdizzy left a comment

dapdizzy Mar 7, 2018

maennchen Mar 7, 2018

maennchen commented Mar 7, 2018

maennchen commented Mar 7, 2018

maennchen commented Mar 7, 2018

dapdizzy commented Mar 7, 2018 via email

maennchen commented Mar 7, 2018

c-rack commented Mar 7, 2018

dapdizzy commented Mar 8, 2018 via email

mertkahyaoglu commented Sep 2, 2018

maennchen commented Sep 2, 2018

mertkahyaoglu commented Sep 3, 2018 •

edited

Loading

dapdizzy commented Sep 4, 2018 •

edited

Loading

maennchen commented Sep 4, 2018

maennchen commented Sep 4, 2018

mertkahyaoglu commented Sep 4, 2018 •

edited

Loading

dapdizzy commented Sep 4, 2018 via email

mertkahyaoglu commented Sep 4, 2018 •

edited

Loading

maennchen commented Sep 4, 2018

maennchen commented Sep 4, 2018

		@@ -34,8 +34,10 @@ defmodule Quantum.JobBroadcaster do

		@doc false

		@@ -153,6 +159,8 @@ defmodule Quantum.JobBroadcaster do

		:ok = storage.purge(scheduler)

		:ok = storage.purge(scheduler)

Storage adapter implementation: Implemented Quantum.Storage.Adapter behavior via PersistentETS. #318

Storage adapter implementation: Implemented Quantum.Storage.Adapter behavior via PersistentETS. #318

Conversation

pyatkov commented Mar 5, 2018

maennchen commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapdizzy Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maennchen commented Mar 5, 2018

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

dapdizzy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maennchen commented Mar 7, 2018

maennchen commented Mar 7, 2018

maennchen commented Mar 7, 2018

dapdizzy commented Mar 7, 2018 via email

maennchen commented Mar 7, 2018

c-rack commented Mar 7, 2018

dapdizzy commented Mar 8, 2018 via email

mertkahyaoglu commented Sep 2, 2018

maennchen commented Sep 2, 2018

mertkahyaoglu commented Sep 3, 2018 • edited Loading

dapdizzy commented Sep 4, 2018 • edited Loading

maennchen commented Sep 4, 2018

maennchen commented Sep 4, 2018

mertkahyaoglu commented Sep 4, 2018 • edited Loading

dapdizzy commented Sep 4, 2018 via email

mertkahyaoglu commented Sep 4, 2018 • edited Loading

maennchen commented Sep 4, 2018

maennchen commented Sep 4, 2018

dapdizzy Mar 7, 2018 •

edited

Loading

mertkahyaoglu commented Sep 3, 2018 •

edited

Loading

dapdizzy commented Sep 4, 2018 •

edited

Loading

mertkahyaoglu commented Sep 4, 2018 •

edited

Loading

mertkahyaoglu commented Sep 4, 2018 •

edited

Loading