nickva commented on code in PR #31:
URL: https://github.com/apache/couchdb-ioq/pull/31#discussion_r3581016601
##########
src/ioq_server2.erl:
##########
@@ -51,14 +51,14 @@
-record(state, {
- reqs :: khash:khash(),
- waiters :: khash:khash(),
+ reqs :: #{any() => ioq_request()},
Review Comment:
These could be type aliases in this module too perhaps `reqs()` and
`waiters()` it just makes the type signatures a bit neater.
Another idea that for plain maps, which we initialize to #{} always, we can
initialize them in the record definition directly. That could eliminate some
dialyzer warning possible since it avoids the intermediate pre-initialized
`undefined` values.
```
reqs = #{} :: ...
waiters = #{} :: ...
```
##########
src/ioq_server2.erl:
##########
@@ -293,8 +287,8 @@ start_link(Name, SID, Bind) ->
init([Name, SID]) ->
{ok, HQ} = hqueue:new(),
- {ok, Reqs} = khash:new(),
- {ok, Waiters} = khash:new(),
+ Reqs = #{},
Review Comment:
If these are defaulted to `#{}` in the record definition we don't have to
worry about them here. We would have to explicitly set `hqueue:new()` though
##########
src/ioq_server2.erl:
##########
@@ -472,15 +465,15 @@ submit_request(Req, #state{iterations=Iterations}=State)
->
couch_stats:increment_counter([couchdb, io_queue2, Class, count]),
couch_stats:increment_counter([couchdb, io_queue2, RW, count]),
couch_stats:update_histogram([couchdb, io_queue2, submit_delay], Latency),
- khash:put(Reqs, Ref, Req#ioq_request{tsub=SubmitTime, ref=Ref}),
- State#state{iterations=Iterations+1}.
+ Reqs = maps:put(Ref, Req#ioq_request{tsub=SubmitTime, ref=Ref},
State#state.reqs),
+ State#state{iterations = Iterations + 1, reqs = Reqs}.
--spec send_response(khash:khash(), ioq_request(), term()) -> [ok].
-send_response(Waiters, #ioq_request{key=Key}, Reply) ->
- Waiting = khash:get(Waiters, Key),
- khash:del(Waiters, Key),
- [gen_server:reply(W, Reply) || W <- Waiting].
+-spec send_response(state(), ioq_request(), term()) -> state().
+send_response(#state{waiters = Waiters} = State0, #ioq_request{key = Key},
Reply) ->
+ {ok, Waiting} = maps:find(Key, Waiters),
Review Comment:
Just a note here that we're asserting the existence of the key a bit earlier
than in the old code. In the old code, if somehow the key was missing we would
have crashed in the `[gen_server:reply(W, Reply) || W <- Waiting]` with a bad
`undefined` generator now we'll crash on badmatch in the `maps:find/2`.
In fact, we should simplify this even more and use `maps:take` to emphasize
the assertion even more
```erlang
{Waiting, Waiters1} = maps:take(Key, Waiters}
...
State0#state{waiters = Waiters1}
```
A tiny bit shorter
##########
src/ioq_server2.erl:
##########
@@ -1095,15 +1093,17 @@ random_server(Servers) ->
lists:nth(rand:uniform(length(Servers)), Servers).
-test_io_error(#state{waiters=Waiters, reqs=Reqs}=State) ->
+test_io_error(#state{waiters=Waiters, reqs=Reqs}=State0) ->
Key = asdf,
Ref = make_ref(),
RefTag = make_ref(),
Req = #ioq_request{ref=Ref, key=Key},
- khash:put(Waiters, Key, [{self(), RefTag}]),
- khash:put(Reqs, Ref, Req),
+ State1 = State0#state{
+ waiters = maps:put(Key, [{self(), RefTag}], Waiters),
Review Comment:
Use `#{Key => ...}`
##########
src/ioq_server2.erl:
##########
@@ -506,20 +499,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 maps:find(ReqKey, State#state.waiters) of
+ error ->
Priority = prioritize_request(Req, State),
Req1 = Req#ioq_request{
key = ReqKey,
init_priority = Priority
},
hqueue:insert(HQ, Priority, Req1),
- khash:put(State#state.waiters, ReqKey, [From]);
- Pids ->
+ maps:put(ReqKey, [From], State#state.waiters);
Review Comment:
To avoid repeating `State#state.waiters` deconstruct #state{queue=HQ,
waiters = Waiters0} = State` at the start of the function. Same idea as above,
match first on general record types then deconstruct right below to pick out
individual variables we need
##########
src/ioq_server2.erl:
##########
@@ -343,32 +337,32 @@ handle_cast(_Msg, State) ->
{noreply, State, 0}.
-handle_info({Ref, Reply}, #state{reqs = Reqs} = State) ->
- case khash:get(Reqs, Ref) of
- undefined ->
- ok;
- #ioq_request{ref=Ref}=Req ->
- ok = khash:del(Reqs, Ref),
+handle_info({Ref, Reply}, #state{reqs = Reqs} = State0) ->
+ State1 = case maps:find(Ref, Reqs) of
+ error ->
+ State0;
+ {ok, #ioq_request{ref = Ref} = Req} ->
TResponse = os:timestamp(),
ServiceTime = time_delta(TResponse, Req#ioq_request.tsub),
IOWait = time_delta(TResponse, Req#ioq_request.t0),
couch_stats:update_histogram(
[couchdb, io_queue2, svctm], ServiceTime),
couch_stats:update_histogram([couchdb, io_queue2, iowait], IOWait),
erlang:demonitor(Ref, [flush]),
- send_response(State#state.waiters, Req, Reply)
+ State = send_response(State0, Req, Reply),
+ State#state{reqs = maps:remove(Ref, Reqs)}
end,
- {noreply, State, 0};
-handle_info({'DOWN', Ref, _, _, Reason}, #state{reqs = Reqs} = State) ->
- case khash:get(Reqs, Ref) of
- undefined ->
- ok;
- #ioq_request{ref=Ref}=Req ->
+ {noreply, State1, 0};
+handle_info({'DOWN', Ref, _, _, Reason}, #state{reqs = Reqs} = State0) ->
Review Comment:
Like above we could split this into two heads one for an element that exist
and one for when it doesn't:
```
handle_info({'DOWN', Ref, _, _, Reason}, #state{reqs = #{Ref := Reqs}} =
State0) ->
...
```
##########
src/ioq_server2.erl:
##########
@@ -343,32 +337,32 @@ handle_cast(_Msg, State) ->
{noreply, State, 0}.
-handle_info({Ref, Reply}, #state{reqs = Reqs} = State) ->
- case khash:get(Reqs, Ref) of
- undefined ->
- ok;
- #ioq_request{ref=Ref}=Req ->
- ok = khash:del(Reqs, Ref),
+handle_info({Ref, Reply}, #state{reqs = Reqs} = State0) ->
Review Comment:
I think we could split it at the stop level into two clauses by matching the
Ref directly.
```erlang
handle_info({Ref, Reply}, #state{reqs = #{Ref := Req}} = State0) ->
% we have the request
;
handle_info({Ref, Reply}, #state{reqs = #{}} = State) ->
% don't have the request
State.
```
Check if the pattern `(...K..., #{K := V})` would work, if it doesn't there
is an `is_map_key(K, Map)` guard that would work
##########
src/ioq_server2.erl:
##########
@@ -472,15 +465,15 @@ submit_request(Req, #state{iterations=Iterations}=State)
->
couch_stats:increment_counter([couchdb, io_queue2, Class, count]),
couch_stats:increment_counter([couchdb, io_queue2, RW, count]),
couch_stats:update_histogram([couchdb, io_queue2, submit_delay], Latency),
- khash:put(Reqs, Ref, Req#ioq_request{tsub=SubmitTime, ref=Ref}),
- State#state{iterations=Iterations+1}.
+ Reqs = maps:put(Ref, Req#ioq_request{tsub=SubmitTime, ref=Ref},
State#state.reqs),
Review Comment:
Since we're deconstructing variables close to the function start, let's
stick with that, otherwise `State#state.reqs` looks a bit out of place.
If the head becomes too long, what I'd do is first match on the general
record shape in the args, and then deconstruct each right below:
```erlang
submit_request(#ioq_request{} = Req, #state{} = State} ->
#ioq_request{fd = ..., } = Req,
#state{reqs = Reqs, iterations = Iterations} = State,
##########
src/ioq_config.erl:
##########
@@ -174,7 +174,7 @@ set_config(Section, Key, Value, Reason) ->
ok = config:set(Section, Key, Value, Reason).
--spec build_shard_priorities() -> {ok, khash:khash()}.
+-spec build_shard_priorities() -> {ok, #{any() => float()}}.
Review Comment:
suggestion nit: define a type alias for the priority map in `ioq.hrl` so it
might be something like `-type ioq_pmap() :: #{any() => float()}.`
##########
src/ioq_server2.erl:
##########
@@ -343,32 +337,32 @@ handle_cast(_Msg, State) ->
{noreply, State, 0}.
-handle_info({Ref, Reply}, #state{reqs = Reqs} = State) ->
- case khash:get(Reqs, Ref) of
- undefined ->
- ok;
- #ioq_request{ref=Ref}=Req ->
- ok = khash:del(Reqs, Ref),
+handle_info({Ref, Reply}, #state{reqs = Reqs} = State0) ->
+ State1 = case maps:find(Ref, Reqs) of
+ error ->
+ State0;
+ {ok, #ioq_request{ref = Ref} = Req} ->
TResponse = os:timestamp(),
ServiceTime = time_delta(TResponse, Req#ioq_request.tsub),
IOWait = time_delta(TResponse, Req#ioq_request.t0),
couch_stats:update_histogram(
[couchdb, io_queue2, svctm], ServiceTime),
couch_stats:update_histogram([couchdb, io_queue2, iowait], IOWait),
erlang:demonitor(Ref, [flush]),
- send_response(State#state.waiters, Req, Reply)
+ State = send_response(State0, Req, Reply),
+ State#state{reqs = maps:remove(Ref, Reqs)}
end,
- {noreply, State, 0};
-handle_info({'DOWN', Ref, _, _, Reason}, #state{reqs = Reqs} = State) ->
- case khash:get(Reqs, Ref) of
- undefined ->
- ok;
- #ioq_request{ref=Ref}=Req ->
+ {noreply, State1, 0};
+handle_info({'DOWN', Ref, _, _, Reason}, #state{reqs = Reqs} = State0) ->
+ State1 = case maps:find(Ref, Reqs) of
+ error ->
+ State0;
+ {ok, #ioq_request{ref = Ref} = Req} ->
couch_stats:increment_counter([couchdb, io_queue2, io_errors]),
- ok = khash:del(Reqs, Ref),
- send_response(State#state.waiters, Req, {'EXIT', Reason})
+ State = send_response(State0, Req, {'EXIT', Reason}),
+ State#state{reqs = maps:remove(Req, Reqs)}
Review Comment:
Here I believe we'd want `maps:remove(Ref, Reqs`) as we're keying requests
by Refs. If we miss this part we could leak requests as maps:remove(Req, Reqs)
would silently fail as a non-op.
##########
src/ioq_server2.erl:
##########
@@ -472,15 +465,15 @@ submit_request(Req, #state{iterations=Iterations}=State)
->
couch_stats:increment_counter([couchdb, io_queue2, Class, count]),
couch_stats:increment_counter([couchdb, io_queue2, RW, count]),
couch_stats:update_histogram([couchdb, io_queue2, submit_delay], Latency),
- khash:put(Reqs, Ref, Req#ioq_request{tsub=SubmitTime, ref=Ref}),
- State#state{iterations=Iterations+1}.
+ Reqs = maps:put(Ref, Req#ioq_request{tsub=SubmitTime, ref=Ref},
State#state.reqs),
Review Comment:
Avoid using `maps:put/2` in general. In the Apache CouchDB code-base we
usually use the syntactic update instead
```erlang
Map#{Key := Val}
```
If we also want to assert that the key should already exist and it's an
update
```erlang
Map#{Key => Val}
```
If the key may not exist and it's an "upsert"
##########
src/ioq_server2.erl:
##########
@@ -506,20 +499,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 maps:find(ReqKey, State#state.waiters) of
Review Comment:
A simpler pattern instead of maps:find is to match the map directly
```erlang
Waiters = case Waiters0 of
#{ReqKey := Pids} -> % found;
#{} -> % not found
end
```
##########
src/ioq_server2.erl:
##########
@@ -506,20 +499,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 maps:find(ReqKey, State#state.waiters) of
+ error ->
Priority = prioritize_request(Req, State),
Req1 = Req#ioq_request{
key = ReqKey,
init_priority = Priority
},
hqueue:insert(HQ, Priority, Req1),
- khash:put(State#state.waiters, ReqKey, [From]);
- Pids ->
+ maps:put(ReqKey, [From], State#state.waiters);
+ {ok, Pids} ->
couch_stats:increment_counter([couchdb, io_queue2, merged]),
- khash:put(Waiters, ReqKey, [From | Pids])
+ maps:put(ReqKey, [From | Pids], State#state.waiters)
Review Comment:
`Waiters0#{ReqKey := [From | Pids]}`
##########
IOQ2.md:
##########
@@ -373,7 +373,7 @@ module and are directly usable for easy testing. You can
also see the full list
of priority values from those priority data structures like so:
```erlang
-([email protected])14> khash:to_list(ShardP).
+([email protected])14> maps:to_list(ShardP).
Review Comment:
Good call updating the readme!
--
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]
