Skip to content

Commit

Permalink
Erase index on crash recovery for any backing queue
Browse files Browse the repository at this point in the history
It is up to the individual implementation to decide what to erase,
i.e. priority queues have one index per priority
  • Loading branch information
Diana Corbacho committed Jun 21, 2016
1 parent 5f1ffc6 commit b2fd873
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 3 deletions.
2 changes: 1 addition & 1 deletion src/rabbit_mirror_queue_slave.erl
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ handle_go(Q = #amqqueue{name = QName}) ->
Self, {rabbit_amqqueue, set_ram_duration_target, [Self]}),
{ok, BQ} = application:get_env(backing_queue_module),
Q1 = Q #amqqueue { pid = QPid },
ok = rabbit_queue_index:erase(QName), %% For crash recovery
_ = BQ:delete_crashed(Q), %% For crash recovery
BQS = bq_init(BQ, Q1, new),
State = #state { q = Q1,
gm = GM,
Expand Down
6 changes: 5 additions & 1 deletion src/rabbit_queue_index.erl
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,11 @@ reset_state(#qistate{ dir = Dir,
undefined -> ok;
_ -> file_handle_cache:close(JournalHdl)
end,
ok = erase_index_dir(Dir),
%% Don't verify return value as it might return an `enoent` error if
%% reset and delete_crashed happen simultaneous. This is easily triggered
%% in priority queues where the master do not wait long enough for all
%% slaves
_ = erase_index_dir(Dir),

This comment has been minimized.

Copy link
@michaelklishin

michaelklishin Jun 27, 2016

Member

This change doesn't seem to be in rabbitmq-server-802. Should I cherry-pick this commit?

This comment has been minimized.

Copy link
@michaelklishin

michaelklishin Jun 27, 2016

Member

716a43b is in rabbitmq-server-802, however.

This comment has been minimized.

Copy link
@dcorbacho

dcorbacho Jun 28, 2016

Contributor

No, ignoring the return value 'hides' the real race condition between stop and start of new processes. Might help in some cases, but the race condition is still there and can cause another errors. The good way to solve this is with the changes on master (which I'm trying to create tests for).

This comment has been minimized.

Copy link
@michaelklishin

michaelklishin Jun 28, 2016

Member

OK, now the difference makes sense. Thank you.

blank_state_dir_funs(Dir, OnSyncFun, OnSyncMsgFun).

init(Name, OnSyncFun, OnSyncMsgFun) ->
Expand Down
2 changes: 1 addition & 1 deletion src/rabbit_variable_queue.erl
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ delete_and_terminate(_Reason, State) ->
a(State2 #vqstate { msg_store_clients = undefined }).

delete_crashed(#amqqueue{name = QName}) ->
ok = rabbit_queue_index:erase(QName).
_ = rabbit_queue_index:erase(QName).

purge(State = #vqstate { len = Len }) ->
case is_pending_ack_empty(State) of
Expand Down

0 comments on commit b2fd873

Please sign in to comment.