Make packet_to_component GD handler more robust #1763

fenek · 2018-03-08T09:56:28Z

mod_global_distrib:get_metadata/2,3 now may return {error, missing_gd_structure} error.
Fixed ETS creation in mod_global_distrib_utils - {heir, none, testing} parameter is illegal.
maybe_update_mapping/2 won't crash now if global_distrib structure is missing in acc.
New integration test is a byproduct of attempts to reproduce the issue; since it covers a new scenario, I haven't removed it.
Fixed component_SUITE:connect_component/2 as it was returning invalid component address.

arcusfelis · 2018-03-08T11:27:45Z

src/global_distrib/mod_global_distrib.erl

@@ -43,15 +43,21 @@ start(Host, Opts0) ->
 stop(Host) ->
    mod_global_distrib_utils:stop(?MODULE, Host, fun stop/0).

-spec get_metadata(mongoose_acc:t(), Key :: term(), Default :: term()) -> Value :: term().
+-spec get_metadata(mongoose_acc:t(), Key :: term(), Default :: term()) ->


Feels like it makes sense to start using {ok, Value} | {error, Reason} return format.

Also, it makes sense to just return Default instead.

Since there's no possibility for error I'd stick with returning a single value. AFAIK that's normal for functions taking a default (proplists:get_value, map:get, application:get_env).

I wanted to keep consistent API for get_ functions... so maybe we can rename them to get_metadata/3 and find_metadata/2, so the API will be similar to map equivalents? Single value returned by /3 and tuple or error for /2.

Sure, go ahead :)

arcusfelis · 2018-03-08T11:31:00Z

src/global_distrib/mod_global_distrib.erl


-spec get_metadata(mongoose_acc:t(), Key :: term()) -> Value :: term().
+-spec get_metadata(mongoose_acc:t(), Key :: term()) ->


tricky error handling is required from the caller of this function.
badkey as an exception, {error, Reason}, high chance that we would not match on {error,_} in the caller code and the tuple would "walk" around our code.

arcusfelis · 2018-03-08T11:40:31Z

src/global_distrib/mod_global_distrib_utils.erl

@@ -145,12 +145,12 @@ create_ets(Names, Type) when is_list(Names) ->
 create_ets(Name, Type) ->
    Self = self(),
    Heir = case whereis(ejabberd_sup) of


not the first time I see this pattern, usually in module start function.
Maybe we should have a dedicated process for keeping our ets tables.
Also, explicit ets:give_away is preferable, because the ets always would be owned by the same process.

mongoose_ets? :) I'll create a task for this.

arcusfelis · 2018-03-08T11:43:26Z

test.disabled/ejabberd_tests/tests/mod_global_distrib_SUITE.erl

+    Component1Config = [{port, 8888}, {component, <<"service1">>} | ComponentCommonConfig],
+    Component2Config = [{port, 9990}, {component, <<"service2">>} | ComponentCommonConfig],
+
+    {Comp1, Addr1, _Name1} = component_SUITE:connect_component(Component1Config),


connect_component should into component_helper. Not in this PR.

I've created a dedicated task for this.

arcusfelis · 2018-03-08T11:44:11Z

test.disabled/ejabberd_tests/tests/component_SUITE.erl

-        {host, ComponentHost} = lists:keyfind(host, 1, ComponentOpts),
-        ComponentAddr = <<ComponentName/binary, ".", ComponentHost/binary>>,
+        {server, ComponentServer} = lists:keyfind(server, 1, ComponentOpts),
+        ComponentAddr = <<ComponentName/binary, ".", ComponentServer/binary>>,
        {Component, ComponentAddr, ComponentName};


map is preferable as a return value (maybe not in this pr).

Can be refactored as a part of "move to helper" PR.

arcusfelis · 2018-03-08T11:49:38Z

test.disabled/ejabberd_tests/tests/mod_global_distrib_SUITE.erl

@@ -833,6 +854,8 @@ get_mapping(Node, Client) ->
    {ok, What} = redis_query(Node, [<<"GET">>, FullJid]),
    What.

+%% Warning! May not work properly with alice or any other user whose
+%% stringprepped JID is different than original one
 delete_mapping(Node, Client) ->


escalus_utils:jid_to_lower(BinJid) can be used to work with all jids.

When I tried to experiment with this function, some tests suddenly broke. :P So I'm putting a warning for now so the PR won't be blocked... but this suite should be refactored definitely.

yep, there is some regex inside, not sure it does normalization too

arcusfelis

Looks good.
I've added some comments.
Ping me if you decide to do/not to do changes, so I can review the final version of the code.

kzemek · 2018-03-09T15:19:57Z

src/global_distrib/mod_global_distrib.erl

@@ -43,15 +43,21 @@ start(Host, Opts0) ->
 stop(Host) ->
    mod_global_distrib_utils:stop(?MODULE, Host, fun stop/0).

-spec get_metadata(mongoose_acc:t(), Key :: term(), Default :: term()) -> Value :: term().
+-spec get_metadata(mongoose_acc:t(), Key :: term(), Default :: term()) ->


Since there's no possibility for error I'd stick with returning a single value. AFAIK that's normal for functions taking a default (proplists:get_value, map:get, application:get_env).

kzemek · 2018-03-09T15:23:40Z

src/global_distrib/mod_global_distrib_utils.erl

-            Origin = mod_global_distrib:get_metadata(Acc, origin),
-            mod_global_distrib_mapping:insert_for_jid(From, Origin);
+            case mod_global_distrib:get_metadata(Acc, origin) of
+                %% Lack of 'global_distrib' indicates 100% local routing...


How did we get to maybe_update_mapping with non-GD message? 😕

When mongoose_router_external* are moved to the beginning of router list and the message is routed locally in cluster, a packet to component will be processed by packet_to_component hook, which is handled by mod_global_distrib_mapping.

Guess I missed the change to the routing list.

The change is done only in one of the deployments, it won't be a default for open source.

arcusfelis

good

Make packet_to_component GD handler more robust

4f046a7

fenek added waiting-for-review global distribution labels Mar 8, 2018

fenek added this to the 3.0.0 milestone Mar 8, 2018

fenek requested review from arcusfelis and kzemek March 8, 2018 09:56

Revert change to delete_mapping in tests

8517ed0

arcusfelis reviewed Mar 8, 2018

View reviewed changes

Unify GD get_metadata API

fa45dfe

kzemek suggested changes Mar 9, 2018

View reviewed changes

Distinguish GD get_metadata/3 and find_metadata/2

2e6ad18

arcusfelis approved these changes Mar 9, 2018

View reviewed changes

fenek merged commit 1605df3 into master Mar 12, 2018

fenek deleted the reorderable-component-routers branch March 12, 2018 07:38

fenek mentioned this pull request Mar 13, 2018

Add missing GD small suite #1770

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make packet_to_component GD handler more robust #1763

Make packet_to_component GD handler more robust #1763

fenek commented Mar 8, 2018 •

edited

Loading

arcusfelis Mar 8, 2018

arcusfelis Mar 8, 2018

kzemek Mar 9, 2018

fenek Mar 9, 2018

kzemek Mar 9, 2018

arcusfelis Mar 8, 2018 •

edited

Loading

arcusfelis Mar 8, 2018

fenek Mar 9, 2018

arcusfelis Mar 8, 2018

fenek Mar 9, 2018

arcusfelis Mar 8, 2018

fenek Mar 9, 2018

arcusfelis Mar 8, 2018 •

edited

Loading

fenek Mar 9, 2018

arcusfelis Mar 9, 2018

arcusfelis left a comment

kzemek Mar 9, 2018

kzemek Mar 9, 2018

fenek Mar 9, 2018

kzemek Mar 9, 2018

fenek Mar 9, 2018

arcusfelis left a comment


		-spec get_metadata(mongoose_acc:t(), Key :: term()) -> Value :: term().
		-spec get_metadata(mongoose_acc:t(), Key :: term()) ->

Make packet_to_component GD handler more robust #1763

Make packet_to_component GD handler more robust #1763

Conversation

fenek commented Mar 8, 2018 • 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

arcusfelis Mar 8, 2018 • 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

arcusfelis Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcusfelis 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

arcusfelis left a comment

Choose a reason for hiding this comment

fenek commented Mar 8, 2018 •

edited

Loading

arcusfelis Mar 8, 2018 •

edited

Loading

arcusfelis Mar 8, 2018 •

edited

Loading