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

Replace shuttle-proxy with redirects #315

Merged
merged 3 commits into from
Jul 19, 2022
Merged

Replace shuttle-proxy with redirects #315

merged 3 commits into from
Jul 19, 2022

Conversation

gmelodie
Copy link
Contributor

@gmelodie gmelodie commented Jul 9, 2022

In a discussion with @jlogelin we realized that what shuttle-proxy did was not very different from what I implemented in #175. I personally think that, at least right now, shuttle-proxy adds an unnecessarily big volume of code, so I just made the main node do what shuttle-proxy did:

When a client hits /content/add or /content/add-car and --disable-content-adding is set on the main node, it fetches available endpoints (other shuttles) and makes the call to them.

Obs: this PR also fixes the biggest issue with #175: users don't need to worry about redirecting now, since the main node just does everything for them transparently.

@gmelodie gmelodie self-assigned this Jul 9, 2022
@alvin-reyes
Copy link
Contributor

Instead of adding it to the main node, we should create a standalone shuttle-proxy that clients can use for their dedicated/self-hosted infra.

I'm thinking we should:
1 - decouple this "code" as a "proxifier" library
2 - create a standalone shuttle-proxy node with this library
3 - use the same proxifier library when a main and shuttle node sets a flag (like: "enable-proxy").

image

@gmelodie
Copy link
Contributor Author

I would agree with that approach if we had to do complicated load balancing, but that's not the case with estuary. It just feels like this is overengineering things, we just need a simple redirect to fix this issue.

alvin-reyes
alvin-reyes previously approved these changes Jul 14, 2022
Copy link
Contributor

@alvin-reyes alvin-reyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We should revisit this later on but we can merge it for now.

@alvin-reyes
Copy link
Contributor

LGTM on the change. Let's just fix the build and any conflicts.

@anjor anjor merged commit 8144a30 into master Jul 19, 2022
@alvin-reyes alvin-reyes deleted the kill-shuttle-proxy branch July 27, 2022 16:02
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.

3 participants