rnewson commented on code in PR #5651:
URL: https://github.com/apache/couchdb/pull/5651#discussion_r2343296562
##########
src/dreyfus/src/dreyfus_rpc.erl:
##########
@@ -44,31 +47,36 @@ call(Fun, DbName, DDoc, IndexName, QueryArgs0) ->
stale = Stale
} = QueryArgs,
{_LastSeq, MinSeq} = calculate_seqs(Db, Stale),
- case dreyfus_index:design_doc_to_index(DDoc, IndexName) of
- {ok, Index} ->
- case dreyfus_index_manager:get_index(DbName, Index) of
- {ok, Pid} ->
- case dreyfus_index:await(Pid, MinSeq) of
- {ok, IndexPid, _Seq} ->
- Result = dreyfus_index:Fun(IndexPid, QueryArgs),
- rexi:reply(Result);
- % obsolete clauses, remove after upgrade
- ok ->
- Result = dreyfus_index:Fun(Pid, QueryArgs),
- rexi:reply(Result);
- {ok, _Seq} ->
- Result = dreyfus_index:Fun(Pid, QueryArgs),
- rexi:reply(Result);
- Error ->
- rexi:reply(Error)
- end;
- Error ->
- rexi:reply(Error)
- end;
+ maybe
+ {ok, Index} ?= dreyfus_index:design_doc_to_index(DDoc, IndexName),
+ {ok, Pid} ?= dreyfus_index_manager:get_index(DbName, Index),
Review Comment:
nice use of maybe.
##########
src/dreyfus/src/dreyfus_rpc.erl:
##########
@@ -44,31 +47,36 @@ call(Fun, DbName, DDoc, IndexName, QueryArgs0) ->
stale = Stale
} = QueryArgs,
{_LastSeq, MinSeq} = calculate_seqs(Db, Stale),
- case dreyfus_index:design_doc_to_index(DDoc, IndexName) of
- {ok, Index} ->
- case dreyfus_index_manager:get_index(DbName, Index) of
- {ok, Pid} ->
- case dreyfus_index:await(Pid, MinSeq) of
- {ok, IndexPid, _Seq} ->
- Result = dreyfus_index:Fun(IndexPid, QueryArgs),
- rexi:reply(Result);
- % obsolete clauses, remove after upgrade
- ok ->
- Result = dreyfus_index:Fun(Pid, QueryArgs),
- rexi:reply(Result);
- {ok, _Seq} ->
- Result = dreyfus_index:Fun(Pid, QueryArgs),
- rexi:reply(Result);
- Error ->
- rexi:reply(Error)
- end;
- Error ->
- rexi:reply(Error)
- end;
+ maybe
+ {ok, Index} ?= dreyfus_index:design_doc_to_index(DDoc, IndexName),
+ {ok, Pid} ?= dreyfus_index_manager:get_index(DbName, Index),
+ try
+ rexi:reply(index_call(Fun, Pid, MinSeq, QueryArgs))
+ catch
+ exit:{noproc, _} ->
+ couch_log:error("Got NOPROC, re-trying", []),
Review Comment:
not sure this log line, with no information about which index etc, is very
useful. I think just omit it. the retry either masks the temporary issue or it
doesn't, and we'll get a reportable error at that point.
##########
src/dreyfus/src/dreyfus_index_manager.erl:
##########
@@ -66,6 +69,14 @@ handle_call({get_index, DbName, #index{sig = Sig} = Index},
From, State) ->
[{_, ExistingPid}] ->
{reply, {ok, ExistingPid}, State}
end;
+handle_call({reopen, DbName, #index{sig = Sig} = Index}, From, State) ->
+ case ets:lookup(?BY_SIG, {DbName, Sig}) of
+ [{_, ExistingPid}] when is_pid(ExistingPid) ->
+ true = ets:delete(?BY_SIG, {DbName, Sig});
+ _ ->
+ ok
+ end,
+ handle_call({get_index, DbName, Index}, From, State);
Review Comment:
This section doesn't reopen the index itself, it only completes part of that
process if it's called in response to a noproc. It should at least kill
ExistingPid imo, but I don't see the point in this clause. Retrying after
catching a noproc should be enough on its own as the exit signal will reach the
index manager gen_server and correctly remove the entries from all ETS tables.
You might need a delay before the retry to make that more likely, though.
--
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]
