-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use monad for handling errors and general flow, also config file #1
base: master
Are you sure you want to change the base?
Conversation
…> write_to_client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments about how the config should be set/read
|
||
config :active_proxy, | ||
timeout: 100, | ||
node1_address: "159.203.44.11" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a list key instead of a indexed key lol
upstream_hosts: [
"159.203.44.11"
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we talked about change address during runtime so I assumed named addresses would be best. Not sure if we still want to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we decide that we didn't want dynamic changes to the upstream hosts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly don't remember.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal for actual storage: store active/inactive data in a persistent KV store instead because it's more flexible. Imagine the case where we decide to have dynamic upstream being added and the proxy restarts. That state is lost and the proxy goes back to relying on the static config. Whereas with the KV store we'd have that state saved. Obviously, this is out of scope for this PR but we should consider this for future active/inactive storage.
For this PR I say you just make a decision of a list or a named active/inactive hosts.
@@ -22,12 +22,16 @@ defmodule ActiveProxy.Proxy do | |||
end | |||
|
|||
defp start_serve_process(socket) do | |||
timeout = Application.get_env(:active_proxy, :timeout) | |||
application_address = Application.get_env(:active_proxy, :node1_address) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you look into if we can have these as constants on the module? and for the module to eval them on startup?
Otherwise, an init on the module should accept a config object and they should be set as constants there
@@ -22,12 +22,16 @@ defmodule ActiveProxy.Proxy do | |||
end | |||
|
|||
defp start_serve_process(socket) do | |||
timeout = Application.get_env(:active_proxy, :timeout) | |||
application_address = Application.get_env(:active_proxy, :node1_address) | |||
Logger.info("Forwarding to application at #{application_address} with timout of #{timeout}ms") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this log to where that goes as well ^
:ok <- :gen_tcp.send(upstream_socket, packet), | ||
{:ok, packet} <- :gen_tcp.recv(upstream_socket, 0, timeout), | ||
:ok <- :gen_tcp.send(client_socket, packet) do | ||
serve(client_socket, upstream_socket, timeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Print this function out and hang it on a wall 😍
What this PR does