Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

vikrambombhi
Copy link
Member

What this PR does

  1. Created a config file with timeout and node address
  2. Cleaned up the flow of serve loop using monad 😩

@vikrambombhi vikrambombhi self-assigned this Dec 5, 2018
Copy link
Member

@JackyChiu JackyChiu left a 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"
Copy link
Member

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"
]

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

@JackyChiu JackyChiu Dec 7, 2018

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)
Copy link
Member

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")
Copy link
Member

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)
Copy link
Member

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 😍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants