Simple client

[laozhp]

Provides a simple client wrapper, that can specify underlying module. Return tagged value {:ok, result} | {:error, reason} | {:exception, thrift_exception}

[scohen]

First off, thanks so much for the contribution, this is really awesome. I especially like that you have tests, I'm a big fan of tests.

Would it be possible to integrate directly with the existing client though? Having the ability to specify the type of client is incredibly useful, but I worry about the diverging implementations of the clients. That, and I think this is worthy of being the default.

The approach that makes the most sense to me is to to provide several options for functions that build clients, and one can use the thrift_client_util approach, and the other use the reconnecting client.

[laozhp]

@scohen I don't know if it can replace the existing client, because the interface has changed.

I make this simple client for two main reason:

• Handle thrift exception using elixir style. That is a XXX function return {:ok, result} | {:error, reason} | {:exception, thrift_exception}, a XXX! function return result direct, and throw/raise on error/exception. The XXX! function is not implemented yet.
• I need a duplex client that can handle request from server using the client transport. I will implement this later.

[scohen]

@laozhp I think I like your client better. If I'm reading this correctly, the interface changes are pretty small and backwards compatible. I think I prefer yours over the existing ones.

[laozhp]

server_test failed with eaddrinuse?

  1) test it should execute successfully with after_start as noop (ServerTest)
     test/riffed/server_test.exs:290
     ** (EXIT from #PID<0.1687.0>) :eaddrinuse

[scohen]

That's common but infrequent, I think there's an issue opened for it; it's due to how the thrift servers shut down, and in code we don't control.

[tudborg]

Was going to implement something like this to handle exceptions. Currently I get a :bad_return_vaue from the gen server when the thrift call throws. Would def. like to see some kind of exception handling merged.

What do you need to be able to merge this? Anything I can do to help?

[scohen]

My main concern was the code duplication between the original client and this one, and wanted to make this the default.

I haven't heard from @laozhp, but I think what needs to happen is to make the bang functions that just return the values directly.

Alternately, if all you care about is the exception handling, I can bake that into the original client.

[tudborg]

A more Elixir-like interface would be nice, but I'm not that bothered by the existing client. But if Riffed just threw exceptions so I could catch them, instead of crashing, that would also be just fine.

I def. agree that there should be one client. And I also agree that @laozhp 's is the ideal choice API-wise.

Maybe a fix to the existing client, so it at least works with thrift exceptions. And then considering @laozhp 's edition in a new and improved client version at some later point. No reason to rush a merge for my sake.

One thing that is currently missing (as far as I can tell) from this PR is to convert thrift exceptions into structs as well. Seems weird to use structs as normal returns, but exceptions are still records.

I'm cool with anything that will add thrift exception handling to Riffed :)

[laozhp]

I have just add the bang functions. Because of thrift's exception is generated to struct in erlang, and can't tell which sturct is exception, information is lost. So riffed can't use defexception to define exception, and then can't use raise/rescue to handle exception. Riffed use throw/catch to handle exception right now, for example:

    try do
      Client.testMultiException!(pid, "Xception", "Message")
    catch
      :throw, {:exception, ex = %SimpleClientModels.Xception{}} ->
        assert ex == SimpleClientModels.Xception.new(
          errorCode: 1001, message: "Xception")
      :throw, {:exception, ex = %SimpleClientModels.Xception2{}} ->
        assert false
    end

[scohen]

@laozhp We should have exception metadata around each call which can be used to dynamically generate a catch clause, no?

FWIW, I don't mind changing the API in the 1.5 release, since we're changing the userland API as well.

BTW, sorry about the latent replies; i'm not getting emails in my inbox.

[scohen]

FYI @laozhp @tbug, I've added exceptions in #20, I'd like your thoughts.

Right now, apache thrift 0.10 is getting ready for beta. I'd like this to target that release, as it makes generation of things much easier. A such, this will land in riffed 1.5, where, I think, we should land @laozhp's client changes to provide a more elixir-like interface with the bang functions. Sound good?