On Thu, Jul 22, 2010 at 4:04 PM, Eugene Letuchy <[email protected]> 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
>> <[email protected]> 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
