Remove send process in ered_connection.erl. #147
Remove send process in ered_connection.erl. #147WilliamVoong wants to merge 10 commits intoEricsson:mainfrom
Conversation
zuiderkwast
left a comment
zuiderkwast
left a comment
There was a problem hiding this comment.
Thanks William. This solves the actually problem, which is the main purpose. I'm posting my comments. Let me think about merging this and doing more stuff later or how to continue. I understand you're busy with other stuff.
This change doesn't actually remove any process. The old send process still exists but it is just doing the connect, starts the recv process and then waits for it to exit. I think we can completely get rid of this process and let ered_connection be only the recv process. The recv process can do the connect. (Also we can turn it into a gen_server, but it might be a larger change that I have some WIP code for.)
src/ered_cluster.erl
Outdated
| command(ClusterRef, Command, Key, Timeout) when is_binary(Key) -> | ||
| C = ered_command:convert_to(Command), | ||
| gen_server:call(ClusterRef, {command, C, Key}, Timeout). | ||
| gen_server:call(ClusterRef, {command, Command, Key}, Timeout). |
There was a problem hiding this comment.
As we talked about: Let's revert these changes. It's good to call convert_to early, before the command is sent between processes. When it's being called again later, it's is a no-op.
src/ered_client.erl
Outdated
| transport_socket = none :: gen_tcp:socket() | ssl:sslsocket(), | ||
| recv_pid = none :: pid(), |
There was a problem hiding this comment.
transport_socket is a tuple so it should have a tuple type like {module(), any()} or more exact {gen_tcp, gen_tcp:socket()} | {ssl, ssl:sslsocket()}.
Additionally, the type needs to allow none because it is set to none by default.
Same with recv_pid. The type needs to be none | pid().
I think lines these should be located just after to the other pids in the state, i.e. just after connect_loop_pid and connection_pid. (What is connect_loop_pid? Is it unused?)
src/ered_client.erl
Outdated
| {noreply, connection_down(Reason, State)}; | ||
|
|
||
| handle_info({connected, Pid, ClusterId}, State) -> | ||
| handle_info({connected, ConnectionPid, Socket, RecvPid, ClusterId}, State) -> |
There was a problem hiding this comment.
What is the connection pid for? I don't think ered_client is using it for anything. We could remove it from this message.
What we need from ered_connection is Transport, Socket and the RecvPid. We could put these three in a tuple and treat it as an opaque connection handle. Technically, ered_client doesn't even need to know what it contains. It could just use it when calling functions in the ered_connection module.
src/ered_client.erl
Outdated
| self() ! {socket_closed, Reason} | ||
| end. | ||
|
|
||
| batch_send(RecvPid, {Socket, Transport}, Commands, Ref) -> |
There was a problem hiding this comment.
This function has duplicated code with send. It's possible to refactor it to avoid the duplicated code.
Additionally, this code that builds RefInfo = {Class, self(), Ref, []} is designed just for the receiving process. Think about this as designing an API between the modules. This RefInfo stuff can be internal details to the ered_connection module. I think the send and batch_send functions should be moved to the ered_connection module, but still run in the caller's process.
src/ered_client.erl
Outdated
| %% This will shut down recv_loop if it is waiting for a reference | ||
| RecvPid ! close_down, | ||
| %% Ok, recv done, time to die | ||
| receive {recv_exit, _Reason} -> ok end, | ||
| self() ! {socket_closed, Reason} |
There was a problem hiding this comment.
"Time to die" is about the ered_connection process. It's not true for the ered_client where this code is running. We should edit the comment or remove it (or move it to ered_connection).
* Get rid of the two processes in ered_connection and duplicated logic. * Use active mode. * Response timeout handling is existing but incomplete. * Batching is not fixed. Only sends one command at a time. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
* Fix unit tests for ered_client. * Fix spelling and formatting. * Chanage ered_parser API to return parse errors instead of throwing them as exceptions. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
* Bonus:
Avoid pattern matching in
eunit tests to verify output/input,
leads to ambigious function clause
errors when clause does not match.
Change-Id: I9e234753c424e520c4d388039919aebc1c1f93f6
2 participants
No description provided.