Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Fix iterator API (1.2 based) #24

Merged
merged 2 commits into from almost 2 years ago

3 participants

Ryan Zezeski Don't Add Me To Your Organization a.k.a The Travis Bot Kelly McLaughlin
Ryan Zezeski
Collaborator

Please see commit comments for details of the issue.

This fix is important because Riak Search handoff cannot complete without it and therefore cluster transitions will never finish.

There are no good work arounds for this issue but if you want handoff to complete you could try one of the following.

  1. Increase the segment_full_read_size--but this only delays the inevitable and causes more memory usage.

  2. Delete the merge index data--this means everything needs to be re-indexed.

rzezeski added some commits
Ryan Zezeski rzezeski Add failing iterator test
Add the iterator command to the EQC test to prove that it is broken.
9b068e8
Ryan Zezeski rzezeski Fix iterator API
The iterator API will timeout any time there is a segment larger than
the `segment_full_read_size`.  In this scenario the segment file is
opened by the `mi_server` process but then reading is attempted by the
spawned iterator process.  Since the segment file is opened in raw mode
this will fail.  The port message will go to the `mi_server` causing
an `Unexpected info...` msg and the iteraor will hit the
`lookup_timeout`.

Many thanks to Arnaud Wetzel who discovered this bug and took time to
explain it to me.
9c2a4b7
Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request passes (merged 9c2a4b7 into d56a99b).

Ryan Zezeski rzezeski referenced this pull request
Closed

Fix iterator API #23

Kelly McLaughlin
Collaborator

Changes look good. Verified that the modified test fails on master and passes on rz-fix-iter-1.2. +1 to merge.

Ryan Zezeski rzezeski merged commit 9c2a4b7 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 2 unique commits by 1 author.

Sep 04, 2012
Ryan Zezeski rzezeski Add failing iterator test
Add the iterator command to the EQC test to prove that it is broken.
9b068e8
Ryan Zezeski rzezeski Fix iterator API
The iterator API will timeout any time there is a segment larger than
the `segment_full_read_size`.  In this scenario the segment file is
opened by the `mi_server` process but then reading is attempted by the
spawned iterator process.  Since the segment file is opened in raw mode
this will fail.  The port message will go to the `mi_server` causing
an `Unexpected info...` msg and the iteraor will hit the
`lookup_timeout`.

Many thanks to Arnaud Wetzel who discovered this bug and took time to
explain it to me.
9c2a4b7
This page is out of date. Refresh to see the latest.
7 src/mi_segment.erl
@@ -154,8 +154,11 @@ iterator(Segment) ->
154 154 false ->
155 155 %% Open a filehandle to the start of the segment.
156 156 {ok, ReadAheadSize} = application:get_env(merge_index, segment_compact_read_ahead_size),
157   - {ok, FH} = file:open(data_file(Segment), [read, raw, binary, {read_ahead, ReadAheadSize}]),
158   - fun() -> iterate_all_filehandle(FH, undefined, undefined) end
  157 + fun() ->
  158 + Opts = [read, raw, binary, {read_ahead, ReadAheadSize}],
  159 + {ok, FH} = file:open(data_file(Segment), Opts),
  160 + iterate_all_filehandle(FH, undefined, undefined)
  161 + end
159 162 end.
160 163
161 164 %% @private Create an iterator over a binary which represents the
6 src/mi_server.erl
@@ -363,8 +363,10 @@ handle_call({iterator, Filter, DestPid, DestRef}, _From, State) ->
363 363 SegmentItrs = [mi_segment:iterator(S) || S <- Segments],
364 364 Itr = build_iterator_tree(BufferItrs ++ SegmentItrs),
365 365
366   - Args = [Filter, DestPid, DestRef, Itr(), {[], 0}],
367   - ItrPid = spawn_link(?MODULE, iterate2, Args),
  366 + ItrPid = spawn_link(
  367 + fun() ->
  368 + iterate2(Filter, DestPid, DestRef, Itr(), {[],0})
  369 + end),
368 370
369 371 NewPids = [ #stream_range{pid=ItrPid,
370 372 caller=DestPid,
12 test/merge_index_tests.erl
@@ -99,6 +99,7 @@ command(S) ->
99 99 %% TODO don't hardcode size to 'all'
100 100 {call,?MODULE,range, [P, g_range_query(Postings), all]},
101 101 {call,?MODULE,range_sync, [P, g_range_query(Postings), all]},
  102 + {call,?MODULE,iterator, [P]},
102 103 {call,?MODULE,drop, [P]},
103 104 {call,?MODULE,compact, [P]}]).
104 105
@@ -149,6 +150,13 @@ postcondition(#state{postings=Postings}, {call,_,fold,_}, {ok, V}) ->
149 150 end,
150 151 ok == ?assert(lists:all(P,V2));
151 152
  153 +postcondition(#state{postings=Postings}, {call,_,iterator,_}, V) ->
  154 + V2 = lists:sort(iterate(V)),
  155 + Postings2 = [{Ii,Ff,Tt,Vv,TS,P} || {Ii,Ff,Tt,Vv,P,TS} <- Postings],
  156 + Postings3 = lists:sort(Postings2),
  157 + P = fun(E) -> lists:member(E, Postings3) end,
  158 + ok == ?assert(lists:all(P, V2));
  159 +
152 160 postcondition(#state{postings=Postings},
153 161 {call,_,lookup,[_,{I,F,T,_,_,_}]}, V) ->
154 162 %% TODO This is actually testing the wrong property b/c there is
@@ -299,6 +307,10 @@ info(Pid, {I,F,T,_,_,_}) ->
299 307 is_empty(Pid) ->
300 308 merge_index:is_empty(Pid).
301 309
  310 +iterator(Pid) ->
  311 + Ft = fun(_,_) -> true end,
  312 + merge_index:iterator(Pid, Ft).
  313 +
302 314 fold(Pid, Fun, Acc) ->
303 315 merge_index:fold(Pid, Fun, Acc).
304 316

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.