Nhse d32 nhskv.i17 (#19)

* Make 2i timeout 503 Avoid issue when an {error, timeout} may be passed as {error, {error, timeout} - resulting in general 500 error * Encoding use JSON library for OTP PR (#20) * Encoding use JSON library for OTP PR Uses the PR'd Json library to OTP for encoding, and passing in a bespoke override on the encode function to avoid the double-pass to convert into a map. This appears to be notably faster than mochijson2 and thoas. * Tidy-up Focus on proposed JSON library for OTP. The riak_kv_clusteraae_fsm has also been reverted back to mochijson2. The format of leveled_tictac:export_tree isn't compatible with the OTP json library - rather than over-complicate the PR, as performance is not critical for these queries, keep the old implementation. * Test integer index * Extend unit test * Switch to raw OTP PR eaed0f7 Without compatibility changes * Correct filename * Compatibility changes to OTP JSON Compatibility changes required for OTP 22 - 26 * Update and include quickcheck properties (#21) * Update and include quickcheck properties * Output too verbose with a collect. * Update comment as per review --------- Co-authored-by: Thomas Arts <[email protected]>
OpenRiak · Mar 28, 2024 · aec41e6 · aec41e6
1 parent b7b690f
commit aec41e6
Show file tree

Hide file tree

Showing 9 changed files with 1,685 additions and 52 deletions.
diff --git a/eqc/json_eqc.erl b/eqc/json_eqc.erl
@@ -0,0 +1,56 @@
+-module(json_eqc).
+
+-include_lib("eqc/include/eqc.hrl").
+-compile([export_all, nowarn_export_all]).
+
+prop_roundtrip() ->
+    in_parallel(?FORALL(Obj, object(), equals(Obj, decode(encode(Obj))))).
+
+prop_escape_all() ->
+    in_parallel(
+    ?FORALL(Str, object(),
+            begin
+               Encoded = iolist_to_binary(encode_escape_all(Str)),
+               all_ascii(Encoded) andalso equals(Str, decode(Encoded))
+            end)).
+
+printable_string() ->
+    Chars = oneof([
+        oneof("\n\r\t\v\b\f\e\d"),
+        choose(16#20, 16#7E),
+        choose(16#A0, 16#D7FF),
+        choose(16#E000, 16#FFFD),
+        choose(16#10000, 16#10FFFF)
+    ]),
+    ?LET(L, list(Chars), unicode:characters_to_binary(L)).
+
+object() -> ?SIZED(Size, object(Size)).
+
+object(0) ->
+    oneof([integer(), real(), printable_string(), bool(), null]);
+object(Size) ->
+    ?LAZY(oneof([object(0),
+                 ?LETSHRINK(Objects, list(object(Size div 4)), Objects),
+                 map(printable_string(), object(Size div 4))
+                ])).
+
+integer() -> choose(-16#8fffffff, 16#ffffffff).
+
+%% Helpers
+
+all_ascii(Bin) ->
+    lists:all(fun(C) -> C < 128 end, binary_to_list(Bin)).
+
+decode(IOList) ->
+    riak_kv_wm_json:decode(iolist_to_binary(IOList)).
+
+%% May produce an iolist
+encode(Str) ->
+    riak_kv_wm_json:encode(Str).
+
+encode_escape_all(Term) ->
+    Encode = fun
+        (Binary, _) when is_binary(Binary) -> riak_kv_wm_json:encode_binary_escape_all(Binary);
+        (Other, Encode) -> riak_kv_wm_json:encode_value(Other, Encode)
+    end,
+    riak_kv_wm_json:encode(Term, Encode).
diff --git a/include/riak_kv_wm_json.hrl b/include/riak_kv_wm_json.hrl
@@ -0,0 +1,189 @@
+%%
+%% %CopyrightBegin%
+%%
+%% Copyright Ericsson AB 2024. All Rights Reserved.
+%%
+%% Licensed under the Apache License, Version 2.0 (the "License");
+%% you may not use this file except in compliance with the License.
+%% You may obtain a copy of the License at
+%%
+%%     http://www.apache.org/licenses/LICENSE-2.0
+%%
+%% Unless required by applicable law or agreed to in writing, software
+%% distributed under the License is distributed on an "AS IS" BASIS,
+%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+%% See the License for the specific language governing permissions and
+%% limitations under the License.
+%%
+%% %CopyrightEnd%
+%%
+
+%% A lot of the macros below use multi-value comparisons where
+%% range checks would have worked just fine. This is because
+%% the compiler & JIT can emit better code in some cases when
+%% multiple clauses are to be dispatched based on such sets
+%% of values. They'll generate an efficient "jump table",
+%% which gets to the correct clause in one go, rather
+%% than going through a set of comparisons.
+%% However, this might not always be the bext way (see is_0_to_9),
+%% so as always with any performance work - measure, don't guess!
+
+-define(is_1_to_9(X),
+    X =:= $1 orelse
+    X =:= $2 orelse
+    X =:= $3 orelse
+    X =:= $4 orelse
+    X =:= $5 orelse
+    X =:= $6 orelse
+    X =:= $7 orelse
+    X =:= $8 orelse
+    X =:= $9
+).
+
+-define(is_0_to_9(X), X >= $0 andalso X =< $9).
+
+-define(is_ws(X), X =:= $\s; X =:= $\t; X =:= $\r; X =:= $\n).
+
+-define(is_ascii_escape(Byte),
+    Byte =:= 0 orelse
+    Byte =:= 1 orelse
+    Byte =:= 2 orelse
+    Byte =:= 3 orelse
+    Byte =:= 4 orelse
+    Byte =:= 5 orelse
+    Byte =:= 6 orelse
+    Byte =:= 7 orelse
+    Byte =:= 8 orelse
+    Byte =:= 9 orelse
+    Byte =:= 10 orelse
+    Byte =:= 11 orelse
+    Byte =:= 12 orelse
+    Byte =:= 13 orelse
+    Byte =:= 14 orelse
+    Byte =:= 15 orelse
+    Byte =:= 16 orelse
+    Byte =:= 17 orelse
+    Byte =:= 18 orelse
+    Byte =:= 19 orelse
+    Byte =:= 20 orelse
+    Byte =:= 21 orelse
+    Byte =:= 22 orelse
+    Byte =:= 23 orelse
+    Byte =:= 24 orelse
+    Byte =:= 25 orelse
+    Byte =:= 26 orelse
+    Byte =:= 27 orelse
+    Byte =:= 28 orelse
+    Byte =:= 29 orelse
+    Byte =:= 30 orelse
+    Byte =:= 31 orelse
+    Byte =:= 34 orelse
+    Byte =:= 92
+).
+-define(is_ascii_plain(Byte),
+    Byte =:= 32 orelse
+    Byte =:= 33 orelse
+    Byte =:= 35 orelse
+    Byte =:= 36 orelse
+    Byte =:= 37 orelse
+    Byte =:= 38 orelse
+    Byte =:= 39 orelse
+    Byte =:= 40 orelse
+    Byte =:= 41 orelse
+    Byte =:= 42 orelse
+    Byte =:= 43 orelse
+    Byte =:= 44 orelse
+    Byte =:= 45 orelse
+    Byte =:= 46 orelse
+    Byte =:= 47 orelse
+    Byte =:= 48 orelse
+    Byte =:= 49 orelse
+    Byte =:= 50 orelse
+    Byte =:= 51 orelse
+    Byte =:= 52 orelse
+    Byte =:= 53 orelse
+    Byte =:= 54 orelse
+    Byte =:= 55 orelse
+    Byte =:= 56 orelse
+    Byte =:= 57 orelse
+    Byte =:= 58 orelse
+    Byte =:= 59 orelse
+    Byte =:= 60 orelse
+    Byte =:= 61 orelse
+    Byte =:= 62 orelse
+    Byte =:= 63 orelse
+    Byte =:= 64 orelse
+    Byte =:= 65 orelse
+    Byte =:= 66 orelse
+    Byte =:= 67 orelse
+    Byte =:= 68 orelse
+    Byte =:= 69 orelse
+    Byte =:= 70 orelse
+    Byte =:= 71 orelse
+    Byte =:= 72 orelse
+    Byte =:= 73 orelse
+    Byte =:= 74 orelse
+    Byte =:= 75 orelse
+    Byte =:= 76 orelse
+    Byte =:= 77 orelse
+    Byte =:= 78 orelse
+    Byte =:= 79 orelse
+    Byte =:= 80 orelse
+    Byte =:= 81 orelse
+    Byte =:= 82 orelse
+    Byte =:= 83 orelse
+    Byte =:= 84 orelse
+    Byte =:= 85 orelse
+    Byte =:= 86 orelse
+    Byte =:= 87 orelse
+    Byte =:= 88 orelse
+    Byte =:= 89 orelse
+    Byte =:= 90 orelse
+    Byte =:= 91 orelse
+    Byte =:= 93 orelse
+    Byte =:= 94 orelse
+    Byte =:= 95 orelse
+    Byte =:= 96 orelse
+    Byte =:= 97 orelse
+    Byte =:= 98 orelse
+    Byte =:= 99 orelse
+    Byte =:= 100 orelse
+    Byte =:= 101 orelse
+    Byte =:= 102 orelse
+    Byte =:= 103 orelse
+    Byte =:= 104 orelse
+    Byte =:= 105 orelse
+    Byte =:= 106 orelse
+    Byte =:= 107 orelse
+    Byte =:= 108 orelse
+    Byte =:= 109 orelse
+    Byte =:= 110 orelse
+    Byte =:= 111 orelse
+    Byte =:= 112 orelse
+    Byte =:= 113 orelse
+    Byte =:= 114 orelse
+    Byte =:= 115 orelse
+    Byte =:= 116 orelse
+    Byte =:= 117 orelse
+    Byte =:= 118 orelse
+    Byte =:= 119 orelse
+    Byte =:= 120 orelse
+    Byte =:= 121 orelse
+    Byte =:= 122 orelse
+    Byte =:= 123 orelse
+    Byte =:= 124 orelse
+    Byte =:= 125 orelse
+    Byte =:= 126 orelse
+    Byte =:= 127
+).
+
+-define(are_all_ascii_plain(B1, B2, B3, B4, B5, B6, B7, B8),
+    (?is_ascii_plain(B1)) andalso
+    (?is_ascii_plain(B2)) andalso
+    (?is_ascii_plain(B3)) andalso
+    (?is_ascii_plain(B4)) andalso
+    (?is_ascii_plain(B5)) andalso
+    (?is_ascii_plain(B6)) andalso
+    (?is_ascii_plain(B7)) andalso
+    (?is_ascii_plain(B8))
+).
diff --git a/priv/riak_kv.schema b/priv/riak_kv.schema
@@ -1517,11 +1517,34 @@
     {default, disabled}
 ]}.
 
+%% @doc Default Secondary Index Timeout (milliseconds)
+%% Set the default timeout for running secondary index queries in millisceconds
+%% This is the time to run the query, and collate the results - the encoding of
+%% those results for dispatch back to the client is outside of the scope of
+%% this timeout.  Setting to 0 (default) implies no timeout.
+%% On hitting the timeout a 503 error will be returned on the HTTP API
+{mapping, "secondary_index_timeout", "riak_kv.secondary_index_timeout", [
+  {datatype, integer},
+  {default, 0}
+]}.
+
+%% @doc Json library for 2i results
+%% Prior to Riak KV 3.2.1 the mochijson2 library was used to json encode 2i
+%% results.  For larger results, the performance of this library is slower
+%% when compared to alternatives (e.g. otp).  A back-ported version of the
+%% OTP 27 proposed Json library is now the default - but may return keys in a
+%% different order (i.e. the continuation key may now be before the results
+%% key).  To reverse back to mochijson, then reset to mochijson here.
+{mapping, "secondary_index_json", "riak_kv.secondary_index_json", [
+  {datatype, {enum, [otp, mochijson]}},
+  {default, otp}
+]}.
+
 %% @doc For $key index queries, should keys which are tombstones be returned.
 %% This config will only make a difference with the leveled backend, it is
 %% ignored on other backends.  Disable to change default behaviour and stop
 %% returning keys of tombstones in $key queries
 {mapping, "dollarkey_readtombs", "riak_kv.dollarkey_readtombs", [
     {datatype, {flag, enabled, disabled}},
     {default, enabled}
-]}.
+]}.
diff --git a/src/riak_client.erl b/src/riak_client.erl
@@ -1187,8 +1187,10 @@ wait_for_query_results(ReqId, Timeout) ->
 wait_for_query_results(ReqId, Timeout, Acc) ->
     receive
         {ReqId, done} -> {ok, lists:flatten(lists:reverse(Acc))};
-        {ReqId,{results, Res}} -> wait_for_query_results(ReqId, Timeout, [Res | Acc]);
-        {ReqId, Error} -> {error, Error}
+        {ReqId, {results, Res}} ->
+            wait_for_query_results(ReqId, Timeout, [Res | Acc]);
+        {ReqId, {error, Error}} -> {error, Error};
+        {ReqId, UnexpectedMsg} -> {error, UnexpectedMsg}
     after Timeout ->
             {error, timeout}
     end.
@@ -1200,7 +1202,8 @@ wait_for_query_results(ReqId, Timeout, Acc) ->
 wait_for_fold_results(ReqId, Timeout) ->
     receive
         {ReqId, {results, Results}} -> {ok, Results};
-        {ReqId, Error} -> {error, Error}
+        {ReqId, {error, Error}} -> {error, Error};
+        {ReqId, UnexpectedMsg} -> {error, UnexpectedMsg}
     after Timeout ->
         {error, timeout}
     end.

diff --git a/src/riak_index.erl b/src/riak_index.erl
@@ -58,7 +58,7 @@
 
 %% See GH610, this default is for backwards compat, so 2i behaves as
 %% it did before the FSM timeout bug was "fixed"
--define(DEFAULT_TIMEOUT, infinity).
+-define(DEFAULT_TIMEOUT, 0).
 
 %% @type data_type_defs()  = [data_type_def()].
 %% @type data_type_def()   = {MatchFunction :: function(), ParseFunction :: function()}.
@@ -503,17 +503,16 @@ decode_continuation(undefined) ->
 decode_continuation(Bin) ->
     binary_to_term(base64:decode(Bin)).
 
-%% @doc add the `timeout' option tuple to the
-%% `Opts' proplist. if `Timeout' is undefined
-%% then the `app.config' property
-%% `{riak_kv, seconady_index timeout}' is used
-%% If that config property is defined, then
-%% a default of `infinity'  is used.
-%% We use `infinity' as the default to
-%% match the behavior pre 1.4
+%% @doc add the `timeout' option tuple to the `Opts' proplist. if `Timeout' is
+%% undefined then the `app.config' property `{riak_kv, seconady_index timeout}'
+%% is used.  If that config property is undefined, then a default of `infinity'
+%% (a timeout of 0 will be translated to infinity) is used.
+%% We use `infinity' as the default to match the behavior pre 1.4
 add_timeout_opt(undefined, Opts) ->
-    Timeout = app_helper:get_env(riak_kv, secondary_index_timeout, ?DEFAULT_TIMEOUT),
-    [{timeout, Timeout} | Opts];
+    Timeout =
+        app_helper:get_env(
+            riak_kv, secondary_index_timeout, ?DEFAULT_TIMEOUT),
+    add_timeout_opt(Timeout, Opts);
 add_timeout_opt(0, Opts) ->
     [{timeout, infinity} | Opts];
 add_timeout_opt(Timeout, Opts) ->

diff --git a/src/riak_kv_clusteraae_fsm.erl b/src/riak_kv_clusteraae_fsm.erl
@@ -1059,4 +1059,3 @@ convert_validate_test() ->
                     [IQ1, IQ2, IQ3, IQ4, IQ5, IQ6, IQ7]).
 
 -endif.
-