Stop search indexer when it is deleted. #16

brkolla · 2017-05-09T12:45:49Z

Allow dreyfus to listen to ddoc_updated events and check if the index
is deleted as a result of update to it's design document. If the index
is deleted then it will stop the index if waitlist is empty, if the
waitlist is not empty then it will wait for a minute before shutting
down.

BugzId:85718

brkolla · 2017-05-10T17:49:51Z

@rnewson Can you please review the dreyfus code change
@iilyak Can you please review the unit tests

iilyak · 2017-05-31T21:13:27Z

src/dreyfus_index_manager.erl

+    DbShards = [mem3:name(Sh) || Sh <- mem3:local_shards(mem3:dbname(DbName))],
+    lists:foreach(fun(DbShard) ->
+        lists:foreach(fun({_DbShard, {_DDocId, Sig}}) ->
+        case ets:lookup(?BY_SIG, {DbShard, Sig}) of


Indent case

iilyak · 2017-05-31T21:13:32Z

src/dreyfus_index_manager.erl

+            [] ->
+                ok
+        end
+    end, ets:match_object(?BY_DB, {DbShard, {DDocId, '$1'}})) end, DbShards),


could you move second end to another line?

iilyak · 2017-05-31T21:33:56Z

src/dreyfus_index_manager.erl

@@ -113,25 +115,48 @@ handle_db_event(DbName, created, _St) ->
 handle_db_event(DbName, deleted, _St) ->
    gen_server:cast(?MODULE, {cleanup, DbName}),
    {ok, nil};
+handle_db_event(DbName, {ddoc_updated, DDocId}, _St) ->


You are subscribing for all_dbs here. This means that you would get non-clustered databases as well. You should distinguish these two cases. Which can be done by matching on <<"shards/", _/binary>> = DbName.

iilyak · 2017-05-31T21:42:11Z

test/search_index_ddoc_update_tests.erl

+    ok = config:set("admins", ?USER, ?PASS, _Persist=false),
+    Ctx.
+
+setup(PortType) ->


dreyfus is not accessible on backdoor interface. Therefore there is no need to pass PortType and use foreachx in the test.

I tried to update this unit test without the PortType and couldn't make it work.. Can you please show me an example unit test code that I can follow? I want to use this in the mean time as it's doing the intended job.

Here is one example of it https://github.com/apache/couchdb/blob/master/src/couch/test/couchdb_compaction_daemon_tests.erl#L62

iilyak · 2017-05-31T21:47:27Z

test/search_index_ddoc_update_tests.erl

+        dreyfus_by_db, {'$1', {DDocId, Sig}}),
+    lists:foldl(fun({DbName, {_DDocId, _Sig}}, Acc) ->
+        case ets:lookup(dreyfus_by_sig, {DbName, Sig}) of
+            [{_, Pid}] -> [Pid| Acc];


There is no guarantee that dreyfus_by_sig ets table contains Pid of an indexer process. There is a case when it would contain a Pid of an opener process see here.
In order to resolve this ambiguity you could use the approach similar to the this.

In this case, I am issuing a search request, before I call this method.. so it would have opened the index pid before it executes this code..

iilyak · 2017-05-31T21:49:01Z

test/search_index_ddoc_update_tests.erl

+        ]},
+    UpdatedDDocJsonObj.
+
+ensureIndexIsOpened(DbName, DDocId, IndexName) ->


we don't use camel case naming convention in erlang

iilyak · 2017-05-31T21:56:31Z

test/search_index_ddoc_update_tests.erl

+              test_request:get(ReqUrl, [?AUTH]),
+          ?assertEqual(200, Status),
+          DDocId = "_design/searchddoc",
+          Sig = ensureIndexIsOpened(DbName, DDocId, "other"),


You want to make sure that the following conditions are satisfied:

the deleted index is not in ets table

the old index Pid is not running after deletion of search index.

Also keep in mind that you would have multiple indexers running (one per database shard).
I would suggest to use Mayya's approach from here

Yes, the code in here is taking care of handling the event in all db shards.

Also, if the index pid is not there in ets means it is not running right? Isn't that enough to verify that the index pid for the old index is not running? What's the scenario where the index pid would not be in ets but would still be running.. I don't think that's possible..

iilyak · 2017-05-31T21:57:36Z

test/search_index_ddoc_update_tests.erl

+    }.
+
+should_stop_indexing_after_deleting_index(PortType, {Host, DbName}) ->
+    {timeout, 60,


Why you had to increase timeout?

I believe the test was failing if I didn't increase timeout.. May be because I am waiting for the index to be closed before I verify the index is indeed closed.. And that wait seems like is causing the problem.

iilyak

I added some comments

brkolla · 2017-05-31T22:00:53Z

Thanks @iilyak. Will check them out and make the suggested changes as needed.

rnewson · 2017-06-03T18:20:53Z

src/dreyfus_index.erl

@@ -40,6 +40,8 @@
    waiting_list=[]
 }).

+-define(INDEX_SHUTDOWN_DELAY, 60000). % 1 minute


needs to be configurable.

looks like we don't have any config for dreyfus, if I make this configurable then do we need to add a new config file?
Not sure if it's worth the effort to make this configurable.

I mean config:get, that's available to all applications.

rnewson · 2017-06-03T18:22:07Z

src/dreyfus_index.erl

+                    false
+            end
+        end,
+        case IndexDeleted of


indentation is misleadingly wrong here.

rnewson · 2017-06-03T18:23:14Z

src/dreyfus_index.erl

@@ -157,9 +159,43 @@ handle_call(info, _From, State) -> % obsolete
    Reply = info_int(State#state.index_pid),
    {reply, Reply, State}.

+handle_cast({ddoc_updated, DDocResult}, State) ->
+    #state{dbname = DbName, index=#index{sig=CurrentSig}, waiting_list = WaitList} = State,
+    IndexDeleted = case DDocResult of


extract this into a function for clarity.

rnewson · 2017-06-03T18:25:10Z

src/dreyfus_index.erl

+            case Index of
+                [] ->
+                    true;
+                _ ->


match something explicitly here, a non-empty list, for example. we don't expect any other result from get_index_with_sig, so don't have clauses for them.

rnewson · 2017-06-03T18:26:18Z

src/dreyfus_index.erl

+                end;
+            false ->
+                {noreply, State}
+        end;


since we always return {noreply, State}, move it to after the case statement so we say it once.

rnewson · 2017-06-03T18:37:08Z

src/dreyfus_index.erl

+            lists:flatmap(
+                fun(IndexName) ->
+                    case (catch design_doc_to_index(Doc, IndexName)) of
+                        {ok, #index{sig=Sig_New}=Index} when Sig_New =:= Sig -> [Index];


I think you could just do {ok, #index{sig=Sig}=Index} here.

I am trying to compare the if the index list has the index matching the Sig that was passed in as a parameter to this function.

rnewson · 2017-06-03T18:40:00Z

src/dreyfus_index_manager.erl

+        lists:foreach(fun({_DbShard, {_DDocId, Sig}}) ->
+            case ets:lookup(?BY_SIG, {DbShard, Sig}) of
+                [{_, IndexPid}] ->
+                    (catch gen_server:cast(IndexPid, {ddoc_updated, DDocResult}));


To be honest, I just copied the code from couch_index_server and hence the catch.. I thought the motivation was to not kill the index manager if there was an error sending message to search index.. But only time this could raise an error is when something really bad happens to the node.. I will take out the exception..

rnewson · 2017-06-03T18:40:22Z

src/dreyfus_index.erl

+            {IndexNames, _} = lists:unzip(IndexList),
+            lists:flatmap(
+                fun(IndexName) ->
+                    case (catch design_doc_to_index(Doc, IndexName)) of


rnewson · 2017-06-03T18:42:56Z

src/dreyfus_index_manager.erl

+                [] ->
+                    ok
+            end
+        end, ets:match_object(?BY_DB, {DbShard, {DDocId, '$1'}}))


what's the $1 for if we don't apply a match condition?

I followed the same concept as in couch_index_server. I thought this would basically find all the search indices opened for the given ddoc matching DDocId. Am I missing something here?

rnewson · 2017-06-03T18:43:02Z

src/dreyfus_index_manager.erl


 delete_from_ets(Pid, DbName, Sig) ->
+    case ets:match_object(?BY_DB, {DbName, {'$1', Sig}}) of


what's the $1 for if we don't apply a match condition?

This is to find the design doc id for a given DB and Search index, so that I can remove it from the dreyfus_by_db ets.

As Robert mentioned this shouldn't be a '$1', but '_'. We need to bind if we want to compare that the values at different positions are the same. For example {'$1', a, '$1'} would match {22, a, 22}, but it wouldn't match {44, a, 32}.

brkolla · 2017-06-05T14:00:21Z

@rnewson @iilyak Have updated the code to reflect the changes as per your suggestions and also replied to your comments. Can you please take another attempt to review?

rnewson · 2017-06-19T08:27:29Z

src/dreyfus_index.erl

@@ -40,6 +40,9 @@
    waiting_list=[]
 }).

+-define(INDEX_SHUTDOWN_DELAY,
+    config:get("dreyfus", "index_shutdown_delay", 60000)). % 1 minute


config:get returns a string, so the default needs to be a string, or use get_integer. And move this to a function please.

brkolla · 2017-06-19T14:44:47Z

@rnewson, Done. Moved the config:get to a function and also changed it to use get_integer.

iilyak · 2017-06-19T17:38:22Z

test/search_index_ddoc_update_tests.erl

+    ?assertEqual(0, length(IndexPidsFromEts)),
+    ok.
+
+getIndex(DbName, DDocId, IndexName) ->


we don't use CamelCase naming in function names. this function would be called get_index.

iilyak · 2017-06-19T17:45:00Z

test/search_index_ddoc_update_tests.erl

+    ?assertEqual(0, length(IndexPidsFromEts)),
+    ok.
+
+getIndex(DbName, DDocId, IndexName) ->


Should we return not_found instead of ok?

Not really.. The intent of that function is that it ensures that the index is closed and returning 'ok only if it succeeds that.. otherwise the assert would fail and cause the test to stop..

iilyak · 2017-06-19T17:48:41Z

test/search_index_ddoc_update_tests.erl

+    ?assert(length(IndexPidsFromEts) > 0),
+    Sig.
+
+ensure_index_is_closed(DbName, DDocId, IndexName, Sig) ->


How do you know that index Pids are actually killed? It seems like you rely on the info from ets. In this case if we would introduce a bug in a Pid accounting in ets tables we wouldn't be able to catch the problem.
You could pass the Pids of indexer instances as an argument to this function or make this check in the test case itself.

iilyak · 2017-06-19T17:49:56Z

test/search_index_ddoc_update_tests.erl

+
+updated_ddoc_json_obj(DbName, DDocId) ->
+    {ok, DDoc} = fabric:open_doc(DbName, <<DDocId/binary>>, [ejson_body, ?ADMIN_CTX]),
+    % {DDocJsonObj} = ?DDOC_UPDATE,


Don't forget to remove commented out code in the final version.

brkolla · 2017-06-20T15:56:17Z

@iilyak Changed the test code to pass the indexpids and ensure that they are closed.

rnewson

There are some substantial issues to address here.

rnewson · 2017-06-29T11:05:34Z

src/dreyfus_index.erl

@@ -40,6 +40,8 @@
    waiting_list=[]
 }).

+-define(INDEX_SHUTDOWN_DELAY, index_shutdown_delay()).


remove this define, just call the function where needed.

rnewson · 2017-06-29T11:07:02Z

src/dreyfus_index.erl

+handle_cast({ddoc_updated, DDocResult}, State) ->
+    #state{index=#index{sig=CurrentSig}, waiting_list = WaitList} = State,
+    case check_if_index_is_deleted(DDocResult, CurrentSig) of
+        true ->


this case statement will crash when the index isn't deleted as you haven't written the false case. I would also suggest collapsing this to one case statement using {check_if_index_is_deleted(DDocResult, CurrentSig), WaitList}

rnewson · 2017-06-29T11:07:41Z

src/dreyfus_index.erl

+            case WaitList of
+                [] ->
+                    self() ! index_deleted;
+                [_Index] ->


this case statement assumes the wait list will have zero or one items, and crashes if there is more than one waiter.

rnewson · 2017-06-29T11:09:05Z

src/dreyfus_index.erl

+                [] ->
+                    self() ! index_deleted;
+                [_Index] ->
+                    erlang:send_after(?INDEX_SHUTDOWN_DELAY, self(), index_deleted)


it would be better to wait until the list is empty. New requests should not be possible if the index has been deleted.

I thought the whole idea of this work is to stop indexing for the deleted indices which could have already triggered and in progress. Agree that there won't be any _search end point requests after the index is deleted. Basically giving 1 minute to complete the pending requests, if they are not done then it will end the request.

rnewson · 2017-06-29T11:11:22Z

src/dreyfus_index.erl

+get_index_with_sig(#doc{body={Fields}}=Doc, Sig) ->
+    RawIndexes = couch_util:get_value(<<"indexes">>, Fields, {[]}),
+    case RawIndexes of
+        {IndexList} when is_list(IndexList) ->


this case statement has one clause, and crashes on anything else. If that's the intent, you don't need a case statement.

rnewson · 2017-06-29T11:13:01Z

src/dreyfus_index.erl

@@ -222,6 +243,35 @@ code_change(_OldVsn, State, _Extra) ->

 % private functions.

+get_index_with_sig(#doc{body={Fields}}=Doc, Sig) ->


This function only looks for an index of given signature in a single design document. A search index might be referred to by multiple design documents.

I didn't know about this case of index could be referred by multiple design documents. If that's the case I have to scan all the design docs in the db and ensure that it's not referred in other docs.

iilyak · 2017-06-29T16:50:49Z

src/dreyfus_index_manager.erl

+                [] ->
+                    ok
+            end
+        end, ets:match_object(?BY_DB, {DbShard, {DDocId, '$1'}}))


replace '$1' with '_'

iilyak · 2017-06-29T16:51:44Z

test/search_index_ddoc_update_tests.erl

+
+get_index_by_sig_from_ets(DDocId, Sig) ->
+    Indexes = ets:match_object(
+        dreyfus_by_db, {'$1', {DDocId, Sig}}),


replace '$1' with '_'

mayya-sharipova · 2017-07-07T19:00:06Z

src/dreyfus_index.erl

+            ok;
+        _ ->
+            ok
+    end.


This part looks good to me

rnewson · 2017-07-11T18:28:32Z

src/dreyfus_index.erl

+            }=State) ->
+    case check_if_index_is_deleted(DbName, DDocResult, Sig) of
+        true ->
+            erlang:send_after(index_shutdown_delay(), self(), index_deleted);


this will crash the gen server as it returns a value that handle_cast does not expect (a TimerRef).

what's the delay for?

the gen server should shut down after serving last request after deletion.

rnewson · 2017-07-11T18:30:09Z

src/dreyfus_index.erl

+    case Reason of
+        {shutdown, ddoc_updated} ->
+            send_all(State#state.waiting_list, ddoc_updated),
+            ok;


the return value of terminate is not relevant, I would leave this ok; out.

rnewson · 2017-07-11T18:31:58Z

src/dreyfus_index.erl

+        {not_found, deleted} ->
+            IndicesWithSig = indices_with_sig_from_other_ddocs(DbName, CurrentSig);
+        {ok, DDoc} ->
+            IndicesWithSig = indices_with_sig(DbName, DDoc, CurrentSig)


this is wrong. we always need to check if any ddoc refers to the index before we can consider that it's deleted.

We are indeed doing that.. Inside the indices_with_sig, it will first check if it's deleted then it will check if it referred from other ddocs

As per the discussion in slack, will rename/reorg this code to make it easy to read..

mayya-sharipova · 2017-07-13T16:13:32Z

src/dreyfus_index.erl

+check_if_index_is_deleted(DbName, DDocResult, CurrentSig) ->
+    case DDocResult of
+        {not_found, deleted} ->
+            IndicesWithSig = indices_with_sig_from_other_ddocs(DbName, CurrentSig);


this indices_with_sig_from_other_ddocs seems to be very expensive, especially doing exactly same operation for all N shards.
I would suggest avoid this check. First, it is expensive. Second, it seems to me that it is not very likely that the customers have exact same indexes for the same db from diff Design docs and using these Design Docs at the same time. Third, even if we close the index used by another ddoc the danger is minimal, if will be reopened with the next query/ken job.

Even if we still want to keep this check it should be initiated from a single process spawn from dreyfus_index_manager and then results passed to all concerning dreyfus_index processes.

@rnewson @brkolla What do you think?

@brkolla Paul D. has commented on my similar code, and it seems that I was wrong. It is common practice to have the same DDocs when updating them. So, please keep the code as before checking on indices_with_sig_from_other_ddocs

K, will leave it as it is then. Thx.

rnewson · 2017-07-18T14:42:13Z

src/dreyfus_index.erl

@@ -157,9 +157,27 @@ handle_call(info, _From, State) -> % obsolete
    Reply = info_int(State#state.index_pid),
    {reply, Reply, State}.

+handle_cast({ddoc_updated, DDocResult},
+            #state{


indent by 4 spaces.

on principle, we only extract terms in the function head if we are matching on them. so pull Sig and DbName out on a separate line. This will also make the function head much smaller and it will fit on one line.

handle_cast({ddoc_updated, DDocResult}, #state{} = State) -> #index{sig = Sig, dbname = DbName} = State#state.index,

rnewson · 2017-07-18T14:47:37Z

src/dreyfus_index.erl

+            }=State) ->
+    case check_if_index_is_deleted(DbName, DDocResult, Sig) of
+        true ->
+            erlang:send_after(index_shutdown_delay(), self(), index_deleted),


think I said it before, but an arbitrary delay doesn't seem write. If a design document is updated such that there is no reference to this index, we should either let any requests to it finish and then delete the index or just delete it. The arbitrary delay is, I'm guessing, intended to avoid the errors caused by deleting an index while it's being queried? If so, we should at least document that it's a bit hackish.

@rnewson what is the way to "let any requests to it finish"? How do we know that requests are finished? check if Waiters == [] will not work for stale requests that don't wait for indexing to catch up.

brkolla · 2017-07-19T21:42:09Z

@rnewson. I have updated the changes to make changes as per your suggestions..
And in regard to the comment quoted by you (see below), I will do whatever you and @mayya-sharipova decide. I can add a comment in the code summarizing the reasoning for our approach.
As I and @mayya-sharipova mentioned there is no easy way to identify the pids representing the end users (and waiting for indexing to catch up) vs the pids initiated from ken. We can add the comment and move on.

think I said it before, but an arbitrary delay doesn't seem write. If a design document is updated such that there is no reference to this index, we should either let any requests to it finish and then delete the index or just delete it. The arbitrary delay is, I'm guessing, intended to avoid the errors caused by deleting an index while it's being queried? If so, we should at least document that it's a bit hackish.

Allow dreyfus to listen to ddoc_updated events and check if the index is deleted as a result of update to it's design document. If the index is deleted then it will stop the index. If there are any pids waiting after 30 seconds, it will send a message 'ddoc_updates' before it stops the index. BugzId:85718

iilyak reviewed May 31, 2017

View reviewed changes

iilyak requested changes May 31, 2017

View reviewed changes

brkolla force-pushed the 85718-stop-search-index-on-delete branch from 0739e23 to 7214672 Compare June 2, 2017 02:00

rnewson reviewed Jun 3, 2017

View reviewed changes

brkolla force-pushed the 85718-stop-search-index-on-delete branch from 7214672 to ddd87b2 Compare June 5, 2017 13:55

brkolla force-pushed the 85718-stop-search-index-on-delete branch from ddd87b2 to 9194218 Compare June 6, 2017 14:03

brkolla force-pushed the 85718-stop-search-index-on-delete branch from 9194218 to 7e0d982 Compare June 15, 2017 14:09

rnewson reviewed Jun 19, 2017

View reviewed changes

brkolla force-pushed the 85718-stop-search-index-on-delete branch from 7e0d982 to 4abddf9 Compare June 19, 2017 14:42

iilyak reviewed Jun 19, 2017

View reviewed changes

brkolla force-pushed the 85718-stop-search-index-on-delete branch from 4abddf9 to c7e1278 Compare June 19, 2017 17:44

iilyak reviewed Jun 19, 2017

View reviewed changes

brkolla force-pushed the 85718-stop-search-index-on-delete branch from c7e1278 to 2db1f44 Compare June 20, 2017 15:53

brkolla force-pushed the 85718-stop-search-index-on-delete branch from 2db1f44 to ca5ab30 Compare June 28, 2017 21:56

rnewson requested changes Jun 29, 2017

View reviewed changes

iilyak reviewed Jun 29, 2017

View reviewed changes

brkolla force-pushed the 85718-stop-search-index-on-delete branch from ca5ab30 to fd8e486 Compare July 7, 2017 14:21

mayya-sharipova reviewed Jul 7, 2017

View reviewed changes

src/dreyfus_index.erl Outdated

ok;

_ ->

ok

end.

Copy link

Contributor

mayya-sharipova Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part looks good to me

brkolla force-pushed the 85718-stop-search-index-on-delete branch from fd8e486 to b5ce340 Compare July 10, 2017 17:40

rnewson requested changes Jul 11, 2017

View reviewed changes

brkolla force-pushed the 85718-stop-search-index-on-delete branch from b5ce340 to aec2784 Compare July 12, 2017 16:13

mayya-sharipova reviewed Jul 13, 2017

View reviewed changes

rnewson reviewed Jul 18, 2017

View reviewed changes

brkolla force-pushed the 85718-stop-search-index-on-delete branch from aec2784 to 10c62ee Compare July 19, 2017 21:33

brkolla force-pushed the 85718-stop-search-index-on-delete branch from 10c62ee to ce30124 Compare July 26, 2017 14:51

mayya-sharipova mentioned this pull request Aug 28, 2017

Stop dreyfus_index processes on ddoc update #21

Open


		delete_from_ets(Pid, DbName, Sig) ->
		case ets:match_object(?BY_DB, {DbName, {'$1', Sig}}) of

		@@ -222,6 +243,35 @@ code_change(_OldVsn, State, _Extra) ->

		% private functions.

		get_index_with_sig(#doc{body={Fields}}=Doc, Sig) ->

Stop search indexer when it is deleted. #16

Are you sure you want to change the base?

Stop search indexer when it is deleted. #16

Conversation

brkolla commented May 9, 2017

brkolla commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iilyak May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brkolla Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iilyak left a comment

Choose a reason for hiding this comment

brkolla commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brkolla commented Jun 5, 2017

Choose a reason for hiding this comment

brkolla commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brkolla commented Jun 20, 2017

rnewson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayya-sharipova Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iilyak May 31, 2017 •

edited

Loading

brkolla Jun 2, 2017 •

edited

Loading

mayya-sharipova Jul 13, 2017 •

edited

Loading

brkolla commented Jul 19, 2017 •

edited

Loading