It's not a good idea to use same function with the different sets of
parameters, even if we separate them using type checks. The function
interface should be transparent and clear for the programmer. But in
the case of different behaviors of the same function you'll have to
look at the function definition every time you use it - to check if it
will work in the right way.
The backwards-incompatible changes may be not so bad, I agree. But
that's up to Apache code policy.
My current fix is a kind of a crutch but at least it won't lead to any
problems. After all, gen_server uses erlang:register() as well, but
just on the lower level.
On Fri, Jul 23, 2010 at 9:07 PM, Eugene Letuchy <eletu...@facebook.com> wrote:
> Oh sorry. Yeah, you're right, removing the guard should work then.
>
> I think making a backwards-incompatible change *or* adding an additional
> start/4 function that calls into the gen_server module to achieve the
> registration behavior are *both* preferable to writing the naive version of
> the register code.
>
> On 7/22/10 6:07 AM, Dmitry Demeshchuk wrote:
>>
>> On Thu, Jul 22, 2010 at 4:04 PM, Eugene Letuchy<eletu...@facebook.com>
>> wrote:
>>>
>>> Consider changing the various guards to:
>>> is_pid(Client) orelse is_atom(Client)
>>
>> As I mentioned above, I could use not atom or pid:
>>
>> {'somen...@somehost', registered_pid_name}
>>
>> The main Erlang credo is "let it fail".
>>
>>>
>>> Also, I think taking advantage of
>>> http://www.erlang.org/doc/man/gen_server.html#start_link-4 is a better
>>> bet
>>> for your registration wrapper. gen_server contains code for handling the
>>> race condition of spawning the client when the register() call fails.
>>
>> Yes, I was thinking about it initially. But then I came with the
>> following problem. These changes will be backwards-incompatible, so
>> people who are using thrift API for now will have to re-write their
>> code.
>> If it is okay for the architects, then registering using gen_server is
>> much better, or course.
>>
>>>
>>> On 7/22/10 1:16 AM, Dmitry Demeshchuk wrote:
>>>>
>>>> Created a ticket in jira:
>>>>
>>>> https://issues.apache.org/jira/browse/THRIFT-825
>>>>
>>>> On Thu, Jul 22, 2010 at 11:35 AM, Dmitry Demeshchuk
>>>> <demeshc...@gmail.com> wrote:
>>>>>
>>>>> Greetings.
>>>>>
>>>>> I noticed a small inconvenience in the thrift_client Erlang module.
>>>>>
>>>>> call(Client, Function, Args)
>>>>> when is_pid(Client), is_atom(Function), is_list(Args) ->
>>>>> case gen_server:call(Client, {call, Function, Args}) of
>>>>> R = {ok, _} -> R;
>>>>> R = {error, _} -> R;
>>>>> {exception, Exception} -> throw(Exception)
>>>>> end.
>>>>>
>>>>> The inconvenience is that we can use atoms as pointers to processes in
>>>>> Erlang.
>>>>>
>>>>> Say, I write the following wrapper:
>>>>>
>>>>> -module(hbase).
>>>>> -export([
>>>>> start_link/0
>>>>> ]).
>>>>>
>>>>> start_link() ->
>>>>> {ok, Client} = thrift_client:start_link("localhost", 9090,
>>>>> hbase_thrift),
>>>>> register(hbase, Client),
>>>>> {ok, Client}.
>>>>>
>>>>>
>>>>> and then I'll want to access hbase like the following:
>>>>>
>>>>> thrift_client:call(hbase, getTableNames, []).
>>>>>
>>>>> but the guard at the code above won't allow me to use this
>>>>> construction. Moreover, we may want to access a pid like this:
>>>>> {Node, Pid}.
>>>>>
>>>>> I'd rather remove the is_pid check totally from the code and also add
>>>>> a possibility to register the thrift client.
>>>>>
>>>>> Here's the full diff:
>>>>>
>>>>> --- thrift_client.erl
>>>>> +++ thrift_client.erl
>>>>> @@ -22,7 +22,7 @@
>>>>> -behaviour(gen_server).
>>>>>
>>>>> %% API
>>>>> --export([start_link/2, start_link/3, start_link/4,
>>>>> +-export([start_link/2, start_link/3, start_link/4, start_link/5,
>>>>> start/3, start/4,
>>>>> call/3, send_call/3, close/1]).
>>>>>
>>>>> @@ -46,6 +46,11 @@
>>>>> start_link(Host, Port, Service) when is_integer(Port),
>>>>> is_atom(Service)
>>>>> ->
>>>>> start_link(Host, Port, Service, []).
>>>>>
>>>>> +start_link(RegisterName, Host, Port, Service, Options) ->
>>>>> + {ok, Client} = start_link(Host, Port, Service, Options),
>>>>> + register(RegisterName, Client),
>>>>> + {ok, Client}.
>>>>> +
>>>>> start_link(Host, Port, Service, Options) ->
>>>>> start(Host, Port, Service, [{monitor, link} | Options]).
>>>>>
>>>>> @@ -141,7 +146,7 @@
>>>>> end.
>>>>>
>>>>> call(Client, Function, Args)
>>>>> - when is_pid(Client), is_atom(Function), is_list(Args) ->
>>>>> + when is_atom(Function), is_list(Args) ->
>>>>> case gen_server:call(Client, {call, Function, Args}) of
>>>>> R = {ok, _} -> R;
>>>>> R = {error, _} -> R;
>>>>> @@ -149,17 +154,17 @@
>>>>> end.
>>>>>
>>>>> cast(Client, Function, Args)
>>>>> - when is_pid(Client), is_atom(Function), is_list(Args) ->
>>>>> + when is_atom(Function), is_list(Args) ->
>>>>> gen_server:cast(Client, {call, Function, Args}).
>>>>>
>>>>> %% Sends a function call but does not read the result. This is useful
>>>>> %% if you're trying to log non-oneway function calls to write-only
>>>>> %% transports like thrift_disk_log_transport.
>>>>> send_call(Client, Function, Args)
>>>>> - when is_pid(Client), is_atom(Function), is_list(Args) ->
>>>>> + when is_atom(Function), is_list(Args) ->
>>>>> gen_server:call(Client, {send_call, Function, Args}).
>>>>>
>>>>> -close(Client) when is_pid(Client) ->
>>>>> +close(Client) ->
>>>>> gen_server:cast(Client, close).
>>>>>
>>>>> %%====================================================================
>>>>>
>>>>>
>>>>> Just in case, attached the diff as well but not sure if this mailing
>>>>> list allows attachments.
>>>>>
>>>>> --
>>>>> Best regards,
>>>>> Dmitry Demeshchuk
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>
--
Best regards,
Dmitry Demeshchuk
