[GitHub] [couchdb] davisp commented on a change in pull request #3144: move remonitor code into DOWN message

[GitBox <gi...@apache.org>]

davisp commented on a change in pull request #3144:
URL: https://github.com/apache/couchdb/pull/3144#discussion_r486562310



##########
File path: src/smoosh/src/smoosh_channel.erl
##########
@@ -122,17 +122,18 @@ handle_info({'DOWN', Ref, _, Job, Reason}, State0) ->
     #state{active=Active0, starting=Starting0} = State,
     case lists:keytake(Job, 2, Active0) of
         {value, {Key, _Pid}, Active1} ->
-            couch_log:warning("exit for compaction of ~p: ~p", [
-                smoosh_utils:stringify(Key), Reason]),
-            {ok, _} = timer:apply_after(5000, smoosh_server, enqueue, [Key]),
-            {noreply, maybe_start_compaction(State#state{active=Active1})};
+            State1 = maybe_remonitor_cpid(State#state{active=Active1}, Key,
+                Reason),
+            {noreply, maybe_start_compaction(State1)};
         false ->
             case lists:keytake(Ref, 1, Starting0) of
                 {value, {_, Key}, Starting1} ->
-                    couch_log:warning("failed to start compaction of ~p: ~p", [
-                        smoosh_utils:stringify(Key), Reason]),
-                    {ok, _} = timer:apply_after(5000, smoosh_server, enqueue, [Key]),
-                    {noreply, maybe_start_compaction(State#state{starting=Starting1})};
+                    couch_log:warning("failed to start compaction of ~p: ~p",
+                        [smoosh_utils:stringify(Key), Reason]),
+                    {ok, _} = timer:apply_after(5000, smoosh_server, enqueue,
+                        [Key]),
+                    {noreply,
+                        maybe_start_compaction(State#state{starting=Starting1})};

Review comment:
       This hunk appears to be an unintended style change? Unless I'm missing something that's changed that's hard to see with the different wrapping lets remove this bit.

##########
File path: src/smoosh/src/smoosh_channel.erl
##########
@@ -275,24 +276,34 @@ start_compact(State, Db) ->
     case smoosh_utils:ignore_db(Db) of
     false ->
         DbPid = couch_db:get_pid(Db),
-        Key = couch_db:name(Db),
-        case couch_db:get_compactor_pid(Db) of
-            nil ->
-                Ref = erlang:monitor(process, DbPid),
-                DbPid ! {'$gen_call', {self(), Ref}, start_compact},
-                State#state{starting=[{Ref, Key}|State#state.starting]};
-            % database is still compacting so we can just monitor the existing
-            % compaction pid
-            CPid ->
-                couch_log:notice("Db ~s continuing compaction",
-                    [smoosh_utils:stringify(Key)]),
-                erlang:monitor(process, CPid),
-                State#state{active=[{Key, CPid}|State#state.active]}
-        end;
+        Ref = erlang:monitor(process, DbPid),
+        DbPid ! {'$gen_call', {self(), Ref}, start_compact},
+        State#state{starting=[{Ref, couch_db:name(Db)}|State#state.starting]};

Review comment:
       I don't think this is correct. A compaction could be running due to manual intervention or perhaps if smoosh crashed and left a compaction running. I'd just change the comment to be something like "Compaction is already running, so monitor existing compaction pid".




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org