noahshaw11 commented on a change in pull request #3766:
URL: https://github.com/apache/couchdb/pull/3766#discussion_r775751772
##########
File path: src/smoosh/src/smoosh_channel.erl
##########
@@ -232,22 +232,24 @@ handle_info(unpause, State0) ->
State = maybe_open_queue(State0),
{noreply, maybe_start_compaction(State#state{paused = false})}.
-terminate(_Reason, #state{name = Name, waiting = Q}) ->
- file:delete(active_file_name(Name)),
- file:delete(starting_file_name(Name)),
- if
- Q =/= nil ->
- smoosh_priority_queue:close(Q);
- true ->
- nil
- end,
+terminate(_Reason, _State) ->
ok.
maybe_recover_state(#state{name = Name} = State) ->
Active = recover(active_file_name(Name)),
Starting = recover(starting_file_name(Name)),
- DatabaseDir = config:get("couchdb", "database_dir"),
- Active1 = get_matching_compact_files(DatabaseDir, Active),
+ Active1 = lists:foldl(
+ (fun({DbName, CPid}, Acc) ->
+ case couch_server:is_compacting(DbName) of
+ true ->
+ [{DbName, CPid} | Acc];
Review comment:
If we call `is_process_alive` for each `CPid`, I believe we will run
into the problem where we desync our `active` in the `state`. For example, if
we found a `CPid` that is not alive and call `enqueue` on the `DbName` for that
`CPid`, it will enqueue and record the `DbName` in `active`. However, at the
end of `maybe_recover_state` we overwrite the existing `active`:
```State#state{active = Active1, starting = Starting}```. This essentially
removes the jobs that we just enqueued. Maybe it would be safer to use the
timestamp approach you recommended but in an "all or nothing" approach where if
the timestamps do not match then we enqueue each DB that we recovered from the
state file. Otherwise we follow the existing approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
