Skip to content

Persisting worklist using CubQ - #46

Open
jfcloutier wants to merge 9 commits into
mainfrom
cubq
Open

Persisting worklist using CubQ#46
jfcloutier wants to merge 9 commits into
mainfrom
cubq

Conversation

@jfcloutier

@jfcloutier jfcloutier commented Oct 25, 2021

Copy link
Copy Markdown
Collaborator

Support pluggable implementations of a work list, one transient (the default) and the other persistent (via CubQ).

Comment thread lib/jackalope/work_list.ex Outdated
Comment thread mix.exs Outdated
Comment thread lib/jackalope/work_list.ex Outdated
Comment thread lib/jackalope/session.ex Outdated
@jfcloutier
jfcloutier marked this pull request as draft December 17, 2021 20:34
@jfcloutier
jfcloutier force-pushed the cubq branch 11 times, most recently from a4c9741 to 6c549aa Compare December 21, 2021 19:47
@jfcloutier
jfcloutier marked this pull request as ready for review December 21, 2021 19:49
@jfcloutier
jfcloutier force-pushed the cubq branch 7 times, most recently from ee35399 to 3453e99 Compare January 4, 2022 15:07
@jfcloutier
jfcloutier force-pushed the cubq branch 9 times, most recently from 4b9dee1 to 0e54ec6 Compare January 7, 2022 14:33
@jfcloutier

Copy link
Copy Markdown
Collaborator Author

Protocol is now used instead of behaviour for "pluggable work list". The persistent work list stores system monotonic time every 10 minutes and on terminate. Expiration is now simply a monotonic time. In the tests, Jackalope is now started supervised and is no longer explicitly stopped.

Comment thread lib/jackalope.ex Outdated
@jfcloutier
jfcloutier force-pushed the cubq branch 4 times, most recently from 84ae713 to f67856e Compare January 10, 2022 19:34
@fhunleth
fhunleth force-pushed the cubq branch 2 times, most recently from ef96d37 to d6fb5ff Compare January 11, 2022 15:26
Comment thread lib/jackalope/persistent_work_list.ex Outdated
Comment on lines +46 to +58
db =
case CubDB.start_link(data_dir: data_dir, name: :work_list, auto_compact: true) do
{:ok, pid} ->
pid

{:error, {:already_started, pid}} ->
pid

other ->
Logger.warn("[Jackalope] Corrupted DB : #{inspect(other)}. Erasing it.")
_ = File.rmdir(data_dir)
raise "Corrupted work list DB"
end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about something like this? The GenServer should never return already_started. It would be nice to more specifically know that the DB is corrupt rather than always erasing, but I didn't look to see if that was possible. Crashing after fixing the corruption just to get init called again seemed like it might have ramifications on sibling GenServers. I didn't want to think about documenting the behavior if the caller used :all_for_one.

Suggested change
db =
case CubDB.start_link(data_dir: data_dir, name: :work_list, auto_compact: true) do
{:ok, pid} ->
pid
{:error, {:already_started, pid}} ->
pid
other ->
Logger.warn("[Jackalope] Corrupted DB : #{inspect(other)}. Erasing it.")
_ = File.rmdir(data_dir)
raise "Corrupted work list DB"
end
cubdb_opts = [data_dir: data_dir, name: :work_list, auto_compact: true]
{:ok, db} =
case CubDB.start_link(cubdb_opts) do
{:error, reason} ->
Logger.warn("[Jackalope] Corrupted DB : #{inspect(reason)}. Erasing it.")
_ = File.rmdir(data_dir)
CubDB.start_link(cubdb_opts)
success -> success
end

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

What if start_link fails for reasons other than DB corruption?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The same thing as before is fine. I.e., crash.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Committed and pushed

Comment thread lib/jackalope/persistent_work_list.ex Outdated
CubDB.set_auto_file_sync(db, true)

queue =
case CubQ.start_link(db: db, queue: queue_name) do

@fhunleth fhunleth Jan 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think things could be shuffled around and the CubDB and CubQ start_links combined in a helper function that would make testing recovery easier.

Comment thread lib/jackalope/persistent_work_list.ex Outdated
Comment on lines +26 to +32
def new(expiration_fn, update_expiration_fn, max_size \\ @default_max_size, opts \\ []) do
args = [
expiration_fn: expiration_fn,
update_expiration_fn: update_expiration_fn,
max_size: max_size,
opts: opts
]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the first three arguments are going to be added to a new argument list anyway, how about passing them in opts from the beginning?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Committed and pushed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants