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

[full-ci] fix: always select next gateway client #10133

Merged
merged 4 commits into from
Sep 23, 2024
Merged

[full-ci] fix: always select next gateway client #10133

merged 4 commits into from
Sep 23, 2024

Conversation

micbar
Copy link
Contributor

@micbar micbar commented Sep 23, 2024

Description

Needed for #10123

@d7oc This is a very important bugfix for kubernetes environments.

Related Issue

  • Fixes <issue_link>

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@mmattel
Copy link
Contributor

mmattel commented Sep 23, 2024

Should we do any docs in the readme about this? (imho yes)

Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

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

Looks ok to me. Please just remove the unused GetCS3apiClient method

services/collaboration/pkg/command/server.go Show resolved Hide resolved
@micbar micbar requested a review from rhafer September 23, 2024 10:58
@jvillafanez
Copy link
Member

I think we could reuse one selector per request. The PR gets a new selector each time we need to contact the gateway, which I'm not sure if it's a good idea. If we want to go this way, a helper method might keep the same complexity.

For example, the RenameFile has several f.gws.Next(), but maybe we only need one for the whole request.

@micbar
Copy link
Contributor Author

micbar commented Sep 23, 2024

I think we could reuse one selector per request. The PR gets a new selector each time we need to contact the gateway, which I'm not sure if it's a good idea. If we want to go this way, a helper method might keep the same complexity.

For example, the RenameFile has several f.gws.Next(), but maybe we only need one for the whole request.

Discussed that with @butonic

we need to do it every time, because a pod can disappear between two requests.

The „good“ solution will be using a client which can do the next selector by itself.

Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

assuming tests pass

Copy link

sonarcloud bot commented Sep 23, 2024

@micbar micbar merged commit 8762607 into master Sep 23, 2024
4 checks passed
@micbar micbar deleted the gtw-selector branch September 23, 2024 15:19
ownclouders pushed a commit that referenced this pull request Sep 23, 2024
[full-ci] fix: always select next gateway client
@jvillafanez
Copy link
Member

The „good“ solution will be using a client which can do the next selector by itself.

Canceling this for now because it seems too much work for little gain.

We'd need to implement the whole GatewayAPIClient, which is huge, just to hide the selector code inside our custom client. I think we should enrich our client with something more in order to make it worthy.

I've tried to include a retry policy in case of network errors so the next request would go to a different gateway, but it seems we don't have guarantees that the next request wouldn't go to the same gateway. Right now, the selector policy is random (we could still hit the same gateway), and even with a round robin policy, since the client is shared, concurrent requests could cycle through the clients, so the retried request might still go to the same gateway.

@micbar
Copy link
Contributor Author

micbar commented Sep 26, 2024

We can just use the go micro client, which is already capable of doing that. And this also applies for http

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.

4 participants