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

feat: Redo endpoint logic (#39) #40

Merged
merged 8 commits into from
Nov 6, 2024
Merged

Conversation

vrongmeal
Copy link
Member

Fixes: #39

Fixes: #39

Signed-off-by: Vaibhav Rabber <[email protected]>
@vrongmeal
Copy link
Member Author

I have added an example for getting endpoints from environment (like we would want to).

examples/basic.rs Outdated Show resolved Hide resolved
examples/basic.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
@shikhar
Copy link
Member

shikhar commented Nov 4, 2024

As a followup, see the note about not including S2-Basin header when it is part of the URI -- it's not a meaningful cost, it's mainly to have a source of continuous validation that we handle it correctly.

@vrongmeal vrongmeal requested a review from a team as a code owner November 5, 2024 08:29
@vrongmeal
Copy link
Member Author

Made the requested changes.

As a followup, see the note about not including S2-Basin header when it is part of the URI -- it's not a meaningful cost, it's mainly to have a source of continuous validation that we handle it correctly.

Can you clarify which note? We don't send the basin header if it's part of the URI, that is handled properly I believe.

Moreover, I was thinking of adding some validation on SDK side to only allow a few valid basin zones after https://github.com/s2-streamstore/s2/pull/395 since setting a basin zone implies we want to send basin name in the URI here.

src/client.rs Outdated Show resolved Hide resolved
Signed-off-by: Vaibhav Rabber <[email protected]>
src/client.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
Copy link
Member

@shikhar shikhar left a comment

Choose a reason for hiding this comment

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

Pushed some tweaks! Feel free to merge if LGTY too. Regarding the additional validation, can be a followup?

@infiniteregrets infiniteregrets changed the title feat: Redo endpoint logic feat: Redo endpoint logic (#39) Nov 6, 2024
@vrongmeal
Copy link
Member Author

Regarding the additional validation, can be a followup?

Yup, once the server side changes are pushed.

@vrongmeal vrongmeal merged commit 54e3615 into main Nov 6, 2024
2 checks passed
@vrongmeal vrongmeal deleted the vrongmeal/endpoint-logic branch November 6, 2024 06:52
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.

Revisit endpoint logic
2 participants