nickva commented on code in PR #31:
URL: https://github.com/apache/couchdb-ioq/pull/31#discussion_r3589402410
##########
src/ioq_server2.erl:
##########
@@ -495,7 +480,7 @@ waiter_key(Req, State) ->
-spec enqueue_request(ioq_request(), state()) -> state().
-enqueue_request(Req, #state{queue=HQ, waiters=Waiters}=State0) ->
+enqueue_request(Req, #state{queue = HQ, waiters = Waiters0} = State0) ->
Review Comment:
This is a tiny nit to increase robustness. We get `Waiters0` from `State0`
but it might be better to get `Waiters0` after the more recent `{ReqKey, State}
= waiter_key(Req, State0)` version of `State`.
This will work fine today as `waiter_key` only updates next_key but if it
somehow updated waiters in the future we could subtly break something
##########
src/ioq_server2.erl:
##########
@@ -68,10 +68,10 @@
max_priority = ?DEFAULT_MAX_PRIORITY :: float()
}).
-
-type state() :: #state{}.
-type waiter_key() :: {pid(), integer()} | pos_integer().
--type priority() :: float(). %% should be non_negative_float().
+-type ioq_requests() :: #{reference() => ioq_request()}.
+-type ioq_waiters() :: #{waiter_key() => [{pid(), reference()}]}.
Review Comment:
Since recently (OTP 24) gen_server `From` are not necessarily references.
There is an opaque variable since it could also an alias. Let's use the
`gen_server:from()` type
##########
src/ioq_server2.erl:
##########
@@ -506,20 +491,20 @@ enqueue_request(Req, #state{queue=HQ,
waiters=Waiters}=State0) ->
couch_stats:increment_counter([couchdb, io_queue2, queued]),
couch_stats:increment_counter([couchdb, io_queue2, RW, queued]),
- case khash:get(State#state.waiters, ReqKey, not_found) of
- not_found ->
+ Waiters = case Waiters0 of
+ #{ReqKey := Pids} ->
+ couch_stats:increment_counter([couchdb, io_queue2, merged]),
+ Waiters0#{ReqKey => [From | Pids]};
Review Comment:
Tiny nit: if we know the key exists (we're in that case branch) let's use
`:=` for the update
##########
src/ioq_server2.erl:
##########
@@ -267,13 +261,13 @@ get_counters() ->
%% @equiv get_state(?SERVER_ID(1))
--spec get_state() -> any().
+-spec get_state() -> state().
get_state() ->
get_state(?SERVER_ID(1)).
-%% Returns a mutated #state{} with list representations of khash/hqueue objects
--spec get_state(atom()) -> any().
+%% Returns a mutated #state{} with list representations of map/hqueue objects
+-spec get_state(atom()) -> state().
Review Comment:
In get_state we don't really return a proper `#state{}` record but something
else a state record but with list kvs instead of maps. The type of `state()` is
`#state{}` so we technically should keep `any()` as the type.
--
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]
