Re: Thrift API for Erlang

[Eugene Letuchy]

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