Accum stage six - is it the last? #1398

bartekgorny · 2017-07-14T12:40:18Z

consistent use of accumulator as a storage (mostly append-only)
extended reach of accumulators along processing chain, as far as it makes sense
stanza/data to be sent passed along an accumulator
cleaned up and refactored internal broadcasting
cleaned up s2s_in, implemented accumulators in this part of chain

Seems this is it...

gadgetci · 2017-07-14T12:41:40Z

apps/ejabberd/src/ejabberd_c2s.erl

+                    {Act, _, NStateData} = send_and_maybe_buffer_stanza(Acc,
+                                                                        {From, To, FinalPacket},
+                                                                        StateData),
+                    case Act of


According to Elvis:

The expression on line 1227 and column 21 is nested beyond the maximum level of 3.

michalwski · 2017-07-14T13:20:08Z

apps/ejabberd/src/ejabberd_c2s.erl

@@ -810,11 +812,17 @@ do_open_session(El, JID, StateData) ->
    ?INFO_MSG("(~w) Opened session for ~s", [StateData#state.socket, jid:to_binary(JID)]),
    Res = jlib:make_result_iq_reply(El),
    Packet = {jid:to_bare(StateData#state.jid), StateData#state.jid, Res},
-    case send_and_maybe_buffer_stanza(Packet, StateData, wait_for_session_or_sm) of
-        {_, _, NStateData, _} ->
+    Acc = mongoose_acc:new(),


From what I see function do_open_session is called only from maybe_open_session. This one is only called from wait_for_session_or_sm. Would it be worth to create the acc (from the stanza user sent) before maybe_open_session is called and pass as an argument?

michalwski · 2017-07-14T13:24:28Z

apps/ejabberd/src/ejabberd_c2s.erl

    % to be removed
    {SendResult, BufferedStateData} =
        send_and_maybe_buffer_stanza({J1, J2, mod_amp:strip_amp_el_from_request(El)}, State),
-    % if we have to check packet before sending we should do it much earlier, when it is
-    % still accumulator
    mod_amp:check_packet(El, result_to_amp_event(SendResult)),


Why the Acc is not passed to mod_amp:check_packet if we have it now?

Because mod_amp doesn't yet know what to do with it. It certainly should - now that we have accumulators mod_amp doesn't need to use stanza to carry internal info. But rewriting mod_amp is beyond the scope of this project.

michalwski · 2017-07-14T13:27:03Z

apps/ejabberd/src/ejabberd_c2s.erl

@@ -3011,6 +3040,9 @@ re_route_packets(Buffer) ->
     || {From, To, Packet} <- lists:reverse(Buffer)],
    ok.

+maybe_enter_resume_session(StateData) ->


Why exactly do we need this helper function?

It makes code cleaner, I guess. In nearly all cases stream management id is taken from state, so there is no need to pass it separately.

michalwski · 2017-07-14T13:28:28Z

apps/ejabberd/src/ejabberd_local.erl

    Acc = mongoose_acc:require(iq_query_info, Acc0),
    IQ = mongoose_acc:get(iq_query_info, Acc),
-    El = mongoose_acc:get(element, Acc),


What's the reasoning behind using element from passed arg and from Acc as before?

I don't understand the question. In Acc we have what came in, and we don't want to change it. When we want to send something out - which may be entirely different thing - we pass it as arg. Hence this change.

Ok, now I get it. Thanks!

michalwski · 2017-07-14T13:35:31Z

apps/ejabberd/src/mongoose_acc.erl

@@ -45,7 +45,7 @@ terminate(M, _F, _L) ->

 terminate(M, received, _F, _L) ->
 %%    ?ERROR_MSG("ZZZ terminate accumulator ~p ~p", [F, L]),


Do we still need this line?

no :) actually I don't think we really need mogoose_acc:terminate anymore.

michalwski

Thanks for the PR! I left some questions in the code.

gadgetci · 2017-07-17T14:30:59Z

apps/ejabberd/src/mongoose_client_api_sse.erl

@@ -37,15 +37,16 @@ handle_notify(_Msg, State) ->
    {nosend, State}.

 handle_info({route, _From, _To, Acc}, State) ->
-    handle_msg(mongoose_acc:get(name, Acc), Acc, State);
+    handle_info({route, _From, _To, Acc, mongoose_acc:get(element, Acc)}, State);


According to Elvis:

Ignored variable is being used on line 40 and column 32.

gadgetci · 2017-07-17T14:31:00Z

apps/ejabberd/src/mongoose_client_api_sse.erl

@@ -37,15 +37,16 @@ handle_notify(_Msg, State) ->
    {nosend, State}.

 handle_info({route, _From, _To, Acc}, State) ->
-    handle_msg(mongoose_acc:get(name, Acc), Acc, State);
+    handle_info({route, _From, _To, Acc, mongoose_acc:get(element, Acc)}, State);


According to Elvis:

Ignored variable is being used on line 40 and column 25.

gadgetci · 2017-09-11T14:24:23Z

apps/ejabberd/src/ejabberd_sm.erl

@@ -964,14 +968,14 @@ process_iq(From, To, Acc, Packet) ->
                    ResIQ = Module:Function(From, To, IQ),


According to Elvis:

Remove the dynamic function call on line 968. Only modules that define callbacks should make dynamic calls.

Wontfix - would require a rewrite of the whole iq handling machinery, which is not within current scope of work.

gadgetci · 2017-09-11T14:24:24Z

apps/ejabberd/src/ejabberd_local.erl

@@ -106,21 +104,19 @@ process_iq(From, To, Acc0) ->
                    if


According to Elvis:

Replace the 'if' expression on line 104 with a 'case' expression or function clauses.

gadgetci · 2017-09-11T14:24:25Z

apps/ejabberd/src/ejabberd_local.erl

@@ -106,21 +104,19 @@ process_iq(From, To, Acc0) ->
                    if


According to Elvis:

The expression on line 104 and column 21 is nested beyond the maximum level of 3.

gadgetci · 2017-09-11T14:24:29Z

apps/ejabberd/src/gen_iq_handler.erl

        {'EXIT', Reason} ->
            ?ERROR_MSG("~p", [Reason]);
-        ResIQ ->
+        {Acc1, ResIQ} ->
            if


According to Elvis:

Replace the 'if' expression on line 146 with a 'case' expression or function clauses.

gadgetci · 2017-09-11T14:24:30Z

apps/ejabberd/src/gen_iq_handler.erl

+                 From :: ejabberd:jid(), To :: ejabberd:jid(), Acc :: mongoose_acc:t(),
+                 IQ :: ejabberd:iq()) -> mongoose_acc:t() | {'error', 'lager_not_running'}.
+process_iq(_Host, Module, Function, From, To, Acc, IQ) ->
+    case catch Module:Function(From, To, Acc, IQ) of


According to Elvis:

Remove the dynamic function call on line 142. Only modules that define callbacks should make dynamic calls.

Wontfix - would require a rewrite of the whole iq handling machinery, which is not within current scope of work.

gadgetci · 2017-09-11T14:24:31Z

apps/ejabberd/src/ejabberd_sm.erl

@@ -964,14 +968,14 @@ process_iq(From, To, Acc, Packet) ->
                    ResIQ = Module:Function(From, To, IQ),
                    if


According to Elvis:

Replace the 'if' expression on line 969 with a 'case' expression or function clauses.

gadgetci · 2017-09-11T14:24:32Z

apps/ejabberd/src/ejabberd_sm.erl

@@ -964,14 +968,14 @@ process_iq(From, To, Acc, Packet) ->
                    ResIQ = Module:Function(From, To, IQ),
                    if


According to Elvis:

The expression on line 969 and column 21 is nested beyond the maximum level of 3.

gadgetci · 2017-09-12T10:12:03Z

apps/ejabberd/src/ejabberd_local.erl

+    Host = To#jid.lserver,
+    case ets:lookup(?IQTABLE, {XMLNS, Host}) of
+        [{_, Module, Function}] ->
+            case Module:Function(From, To, IQ) of


According to Elvis:

Remove the dynamic function call on line 104. Only modules that define callbacks should make dynamic calls.

gadgetci · 2017-09-12T15:37:48Z

apps/ejabberd/src/ejabberd_sm.erl

+    Host = To#jid.lserver,
+    case ets:lookup(sm_iqtable, {XMLNS, Host}) of
+        [{_, Module, Function}] ->
+            case Module:Function(From, To, IQ) of


According to Elvis:

Remove the dynamic function call on line 969. Only modules that define callbacks should make dynamic calls.

…l handlers; now we can start removing mod_amp hacks

…usages of 'to_send', hope to get rid of it altogether; also improved naming somewhat

…t are not stripped when shipping acc to another process)

…nza or filling acc with extra data)

fenek · 2017-10-02T12:59:48Z

test.disabled/ejabberd_tests/tests/acc_e2e_SUITE_data/acc_test_helper.erl

+
+
+test_save_acc(#{type := <<"chat">>} = Acc, _State) ->
+    random:seed(erlang:phash2([node()]),


random is deprecated. Use rand instead.

fenek · 2017-10-02T13:35:09Z

doc/developers-guide/Hooks-and-handlers.md

@@ -58,7 +58,7 @@ In between the XMPP domain (`StateData#state.server`) and handler arguments the
 All handlers attached to the hook have to accept a list as a first argument and also return a list - possibly the same list
 plus one or more items.

-Note, that `ejabberd_hooks:run/3` and `ejabberd_hooks:run_fold/4` **are not interchangeable**.
+Note that `ejabberd_hooks:run/3` and `ejabberd_hooks:run_fold/4` **are not interchangeable**.


This is no longer true, as run actually executes run_fold. In general, I think it is a good idea to update whole document and remove examples and descriptions with run.

fenek · 2017-10-02T14:02:28Z

doc/developers-guide/Hooks-and-handlers.md

 * take return value from the acc the hook call returned
 * pass the modified accumulator on

+Handlers should store their return values in the accumulator; there are three ways to do it:
+* if it is a one-off value which is to be discarded later use `mongoose_acc:put(result, Value, Acc)`


When is "later"?

fenek · 2017-10-02T14:03:42Z

doc/developers-guide/accumulators.md

@@ -1,10 +1,10 @@
 # Accumulators

-XMPP stanza processing starts in the `ejabberd_c2s` module, which receives the stanza from a socket.
+XMPP stanza processing starts in the `ejabberd_c2s` module, which receives the stanza from a socket, or in `ejabberd_s2s_in` which receives stanzas from other MIM clusters.


(...) from federated XMPP clusters.

fenek · 2017-10-02T14:04:21Z

doc/developers-guide/accumulators.md

 The stanza is processed and eventually it and/or other messages are sent out, either to the original sender, to another c2s process within the same MongooseIM installation, or to another XMPP server.

-At the beginning of the main processing chain, an accumulator is created containing the stanza and other values.
-It is then passed through all the stages, until it reaches the end of its life.
+At the beginning of the main processing chain an accumulator is created containing the stanza and other values.


What values? Would be cool to enumerate basic fields available by default.

fenek · 2017-10-02T14:21:13Z

apps/ejabberd/src/mongoose_client_api_sse.erl

@@ -37,15 +37,17 @@ handle_notify(_Msg, State) ->
    {nosend, State}.

 handle_info({route, _From, _To, Acc}, State) ->
-    handle_msg(mongoose_acc:get(name, Acc), Acc, State);
+    handle_msg(mongoose_acc:get(name, Acc), Acc, mongoose_acc:get(element, Acc), State);
+%%    handle_info({route, From, To, Acc, mongoose_acc:get(element, Acc)}, State);


Ouch, should we leave commented code like this? :) With hooks recording it is quite understandable but not here.

fenek · 2017-10-02T14:30:43Z

apps/ejabberd/src/mongoose_acc.erl

    maps:put(Key, Val, Acc).

 -spec get(any()|[any()], t()) -> any().
-get(to_send, Acc) ->
-    get(to_send, Acc, get(element, Acc));


I'm pretty sure that getting element as fallback was needed by Konrad?

fenek · 2017-10-02T14:31:53Z

apps/ejabberd/src/mongoose_acc.erl

+    % read-only; accumulator is for accumulating)
+    case maps:is_key(Key, Acc) of
+        true ->
+%%            ?WARNING_MSG("Overwriting existing key \"~p\" in accumulator,"


Docs state that warning will be logged...

fenek · 2017-10-02T14:33:09Z

apps/ejabberd/src/mongoose_acc.erl

 -spec strip(t()) -> t().
 strip(Acc) ->
-    El = get(to_send, Acc),
+    strip(Acc, get(element, Acc)).


What is the meaning of send_result then, if only element remains after stripping?

There are more things that remain:
[ref, timestamp, from, from_jid, to, to_jid, persistent_properties]
Should send_result be forwarded too? I don't know, we can start an interesting discussion.

fenek · 2017-10-02T14:35:39Z

apps/ejabberd/src/mongoose_acc.erl

+from_element(El) when is_tuple(El) ->
+    Name = <<"broadcast">>,
+    Type = element(1, El),
+    % ref and timestamp will be filled in by strip/2


Why shouldn't we fill them here?

This is a very specific function. Normally from_element is called when we are just starting stanza processing, so it creates a new accumulator and consequently have to generate ref and timestamp. This one is always called when a stanza triggers a broadcast, it is always called from strip, which means we already have an accumulator with ref and timestamp.

goddammit · 2017-10-05T11:30:50Z

doc/developers-guide/Hooks-and-handlers.md

-Apart from being able to notify the rest of the system that some event has happened by running a hook with `ejabberd_hooks:run/3`, it's also possible to [fold](#sidenote-folds) over a sequence of handlers with `ejabberd_hooks:run_fold/4`.
-Like an ordinary `lists:foldl/3`, `ejabberd_hooks:run_fold/4` also requires an initial value to be passed to the function.
-
+Hook handlers are called by "folding", meaning iterating over the list of handlers passing them a set of arguments and some initial value which gets modified and returned by every handler.


Hook handlers are called by "folding".
This means that each handler on a list is passed a set of arguments and an initial value that it then modifies, returns and hands over to the next handler in line.

goddammit · 2017-10-05T11:43:31Z

doc/developers-guide/Hooks-and-handlers.md

@@ -54,13 +53,10 @@ ListOfSomething = ejabberd_hooks:run_fold(a_certain_hook,
                                           StateData#state.server])
 ```

-In between the XMPP domain (`StateData#state.server`) and handler arguments the initial value of the accumulator being passed through the sequence of handlers is inserted - in this case an empty list (`[]`).
-All handlers attached to the hook have to accept a list as a first argument and also return a list - possibly the same list
+In between the XMPP domain (`StateData#state.server`) and handler arguments is the initial value of the accumulator being passed through the sequence of handlers is inserted - in this case an empty list (`[]`).


The initial value of the accumulator being passed through the sequence of handlers (in this case an empty list ([])) is inserted between the XMPP domain (StateData#state.server) and handler arguments.

goddammit · 2017-10-05T11:44:39Z

doc/developers-guide/Hooks-and-handlers.md

-has `get/2`, `get/3`, `put/3` etc.
-It is instantiated with an incoming stanza, passed along throughout the processing
-chain, supplied to and returned from hook calls, and terminated when stanza is leaving MongooseIM.
+MongooseIM uses a dedicated data structure to accumulate results(see ["Accumulators"](accumulators.md)).


MongooseIM uses a dedicated data structure to accumulate results (see "Accumulators").

goddammit · 2017-10-05T11:44:54Z

doc/developers-guide/Hooks-and-handlers.md

-It is instantiated with an incoming stanza, passed along throughout the processing
-chain, supplied to and returned from hook calls, and terminated when stanza is leaving MongooseIM.
+MongooseIM uses a dedicated data structure to accumulate results(see ["Accumulators"](accumulators.md)).
+This data structure is implemented in `mongoose_acc` module, which has a map-like interface: it has `get/2`, `get/3`, `put/3` etc.


This data structure is implemented in the mongoose_acc module, which has a map-like interface: it has get/2, get/3, put/3 etc.

goddammit · 2017-10-05T11:45:54Z

doc/developers-guide/Hooks-and-handlers.md

@@ -91,9 +80,9 @@ The right way to use hooks is therefore:
 * pass the modified accumulator on

 Handlers should store their return values in the accumulator; there are three ways to do it:
-* if it is a one-off value which is to be discarded later use `mongoose_acc:put(result, Value, Acc)`
+* if it is a one-off value whch doesn't need to be passed on along with the accumulator (can be overwritten any time), `mongoose_acc:put(result, Value, Acc)`


if it is a one-off value whch doesn't need to be passed on along with the accumulator (can be overwritten any time), use mongoose_acc:put(result, Value, Acc)

goddammit · 2017-10-05T11:50:32Z

doc/developers-guide/accumulators.md

+* name - <<"message">>, <<"presence">> or <<"iq">>
+* type - e.g. <<"set">> (in iq)
+* attrs - attributs of the root xml element of stanza
+* from, to - full jids of sender and recipient in binary form


from, to - full jids of the sender and recipient in a binary form

goddammit · 2017-10-05T11:51:16Z

doc/developers-guide/accumulators.md

@@ -61,6 +71,10 @@ Acc2 = mongoose_acc:add_prop(myprop, 123, Acc1),
 V = mongoose_acc:get_prop(myprop, Acc2).
 ```

+The rationale behind stripping an accumulator is that many values store in it are context-dependend.


The rationale behind stripping an accumulator is that many values stored in it are context-dependend.

goddammit · 2017-10-05T11:54:08Z

doc/developers-guide/accumulators.md

@@ -61,6 +71,10 @@ Acc2 = mongoose_acc:add_prop(myprop, 123, Acc1),
 V = mongoose_acc:get_prop(myprop, Acc2).
 ```

+The rationale behind stripping an accumulator is that many values store in it are context-dependend.
+For example, `user` and `server` refer to the owner of the c2s process, in another one user will be different (and sometimes server).


For example, in a c2s process user and server refer to its owner, while elsewhere the user will be different (or sometimes even a server). ?

goddammit · 2017-10-05T11:55:32Z

doc/developers-guide/accumulators.md

@@ -103,7 +117,7 @@ The notion of an exit point is a bit vague, because sending a stanza out of Mong
 A single stanza entering the system creates an acc, but then may result in multiple stanzas going out - for example changing a privacy list triggers presence updates and a roster push.
 It is in fact hard to determine when the process has really been finished.
 The approach is to pass along one acc and, when something is just about to be sent out, either send a text and return the acc or pass on a clone of the acc and return the original one.
-This gives us an option to record a fact that a stanza has been sent out.
+This gives us an option to record a fact that a stanza has been sent out: every sending function calls `mongoose_acc:record_sending`, which gives you an opportunity to consult `send_result` key to check what send attempts have been made and with what effect.


This gives us an option to record a fact that a stanza has been sent out.
Every sending function calls mongoose_acc:record_sending, which gives you an opportunity to consult the send_result key to check what send attempts have been made and with what effect.

goddammit · 2017-10-05T11:56:12Z

doc/developers-guide/accumulators.md

-another fresh accumulator out of the data to be sent, stripping it of all caches and the likes but preserving original ref and
-timestamp (`mongoose_acc:strip/2`), or stanza is stored as offline, or is sent out to another XMPP server via `ejabberd_s2s`.
+Eventually there are three things that may happen:
+* the accumulator is sent to another c2s process, pubsub node or muc room - in this case we produce another fresh accumulator out of the data to be sent, stripping it of all caches and the likes but preserving original ref and timestamp and those attrs that should be carried on (`mongoose_acc:strip/2`)


the accumulator is sent to another c2s process, pubsub node or muc room - in this case we produce another fresh accumulator out of the data to be sent, stripping it of all caches and the likes but preserving the original ref and timestamp and those attrs that should be carried on (mongoose_acc:strip/2)

deadjdona · 2017-10-25T20:35:13Z

doc/developers-guide/Hooks-and-handlers.md

 Under the hood it calls the same handlers with an empty accumulator.
 This is deprecated and some day will be removed.

 ### Error handling in hooks

 Hooks are meant to decouple modules; in other words, the caller signals that some event took place or that it intends to use a certain feature or a set of features, but how and if those features are implemented is beyond its interest.
-Hooks don't therefore use the "let it crash" approach. Instead it is rather like "fire-and-forget", more similar in principle to the `Pid ! signal` way.
+Fro that reason hook don't use the "let it crash" approach. Instead it is rather like "fire-and-forget", more similar in principle to the `Pid ! signal` way.


For that reason..

gadgetci reviewed Jul 14, 2017

View reviewed changes

michalwski reviewed Jul 14, 2017

View reviewed changes

gadgetci reviewed Jul 17, 2017

View reviewed changes

fenek added the WIP 🚧 label Aug 16, 2017

bartekgorny force-pushed the accum-stage-six branch from 0bd93ee to 4d2ba09 Compare September 1, 2017 09:31

gadgetci reviewed Sep 11, 2017

View reviewed changes

gadgetci reviewed Sep 12, 2017

View reviewed changes

bartekgorny force-pushed the accum-stage-six branch 2 times, most recently from 408ce22 to 89f5e80 Compare September 12, 2017 15:35

gadgetci reviewed Sep 12, 2017

View reviewed changes

bartekgorny force-pushed the accum-stage-six branch from 89f5e80 to fd496fd Compare September 13, 2017 09:26

bartekgorny force-pushed the accum-stage-six branch from fd496fd to ce9d0b9 Compare September 21, 2017 14:00

fenek added this to the MongooseIM 2.1.0 milestone Sep 25, 2017

bartekgorny added 5 commits September 26, 2017 11:06

treat accumulator keys as mostly-read-only

450d41f

pushed acc down to send_and_maybe_buffer_stanza, returned to top-leve…

3774c0f

…l handlers; now we can start removing mod_amp hacks

pass xmlel along with the acc to process_packet - removed nearly all …

b9b9070

…usages of 'to_send', hope to get rid of it altogether; also improved naming somewhat

inter-process broadcasting rewritten; and, no more to_send stuff

e85713b

cleanup and Elvis-compliance of part of s2s_in

4088441

bartekgorny added 18 commits September 26, 2017 11:07

initialise accumulator upon stanza entering s2s

c1e0369

for elvis

3507bcf

bugfixes

91f16dd

rewrote wait_for_session_or_sm to use acc

f9558af

got rid if mongoose_acc:initialise and mongoose_acc:terminate

e9c1df7

redundant helper

bc4b93a

rewrote wait_for_auth to squash bug in wait_for_sessio_or_sm

1950d61

fixed a horrible bug in acc:from_element

7034769

Reworked handling routed broadcast

362ebc1

started rewriting iq-handlers

f5a7784

route_iq, by mod_ping and mod_caps

267d765

added Acc to iq handler API

8bce368

elvis-compliance and some bugfixes

568b566

add an API for setting persistent properties on accumulator (such tha…

c249108

…t are not stripped when shipping acc to another process)

add preprocessing hook to facilitate customisation (like tweaking sta…

7ad9e7e

…nza or filling acc with extra data)

upgraded mod_offline

be3108a

first end-to-end test of accumulators

91434a2

updated docs, some style fixes

e502d74

bartekgorny force-pushed the accum-stage-six branch from ce9d0b9 to e502d74 Compare September 26, 2017 09:07

fenek requested changes Oct 2, 2017

View reviewed changes

changes as requestd

dfa902d

bartekgorny force-pushed the accum-stage-six branch from 96831c6 to dfa902d Compare October 4, 2017 13:49

goddammit suggested changes Oct 5, 2017

View reviewed changes

[skip ci] linguistical improvements

5f32a16

fenek merged commit 1372967 into master Oct 5, 2017

fenek deleted the accum-stage-six branch October 5, 2017 15:18

deadjdona reviewed Oct 25, 2017

View reviewed changes

		@@ -45,7 +45,7 @@ terminate(M, _F, _L) ->

		terminate(M, received, _F, _L) ->
		%% ?ERROR_MSG("ZZZ terminate accumulator ~p ~p", [F, L]),

		@@ -964,14 +968,14 @@ process_iq(From, To, Acc, Packet) ->
		ResIQ = Module:Function(From, To, IQ),



		test_save_acc(#{type := <<"chat">>} = Acc, _State) ->
		random:seed(erlang:phash2([node()]),

Accum stage six - is it the last? #1398

Accum stage six - is it the last? #1398

Conversation

bartekgorny commented Jul 14, 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

michalwski 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

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