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.

Sign up for GitHub

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

Jump to bottom

[full-ci][tests-only] Enable async uploads #5268

Merged
merged 2 commits into from
Dec 30, 2022
Merged

[full-ci][tests-only] Enable async uploads #5268

merged 2 commits into from
Dec 30, 2022

Conversation

saw-jan
Copy link
Member

@saw-jan saw-jan commented Dec 21, 2022 •
edited
Loading

Description

Enabled async uploads in oCIS server for all the test pipelines

"STORAGE_USERS_OCIS_ASYNC_UPLOADS": True,
"OCIS_EVENTS_ENABLE_TLS": False,

Related Issue

Part of #4095

Motivation and Context

How Has This Been Tested?

  • test environment:

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:

@saw-jan saw-jan self-assigned this Dec 21, 2022
@saw-jan saw-jan changed the base branch from stable-2.0 to master December 22, 2022 03:51
@saw-jan saw-jan force-pushed the tests/enable-async-uploads branch 5 times, most recently from 0c1eeae to 366e32a Compare December 22, 2022 09:28
@kobergj kobergj mentioned this pull request Dec 22, 2022
Closed
@saw-jan saw-jan force-pushed the tests/enable-async-uploads branch 4 times, most recently from 552c9e7 to 4684354 Compare December 27, 2022 04:26
@saw-jan saw-jan marked this pull request as ready for review December 27, 2022 05:22
@saw-jan saw-jan requested a review from kobergj December 27, 2022 05:24
kobergj
kobergj reviewed Dec 27, 2022
View reviewed changes
Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

@saw-jan Very good work, thanks so much! I think we cannot merge it yet since it switches pipeline completely to async uploads.

@micbar How do we want to proceed with this topic? Have an extra pipeline testing async maybe?

@saw-jan saw-jan marked this pull request as draft December 27, 2022 11:23
@micbar
Copy link
Contributor

micbar commented Dec 28, 2022

I would fix the two expected failures and set async as default.

@micbar
Copy link
Contributor

micbar commented Dec 29, 2022

Wohoo!
Needs a reva update and we are good to go.

@kobergj kobergj marked this pull request as ready for review December 29, 2022 10:43
phil-davis
phil-davis approved these changes Dec 29, 2022
View reviewed changes
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

LGTM - the test scenarios send API requests just like they always do, but the server will return the response quickly, and do other async background processing later. This gives the potential for subsequent requests in a test scenario to get a 425 response, indicating that the async processing is still in progress. The test code already handles this and waits and retries when it gets a 425.

@phil-davis
Copy link
Contributor

@kobergj this needs go.sum conflict resolved - there was another PR that b umped reva and touched that.

saw-jan and others added 2 commits December 30, 2022 09:25
@saw-jan @kobergj
enable async uploads
94113b2 
enable async uploads
@kobergj
bump reva
8f48242 
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@phil-davis
Copy link
Contributor

I restarted CI - a few pipelines had slow pull of docker images.

@sonarcloud
Copy link

sonarcloud bot commented Dec 30, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@phil-davis
Copy link
Contributor

@kobergj I will let you coordinate merging this.

I see that there is also PR #5310 by @fschade - so I guess that one of these should be merged first, and then rebase the other.

@kobergj kobergj merged commit f42b59a into master Dec 30, 2022
@delete-merged-branch delete-merged-branch bot deleted the tests/enable-async-uploads branch December 30, 2022 09:42
@phil-davis
Copy link
Contributor

Note: this is not being ported back to stable-2.0 - it is a feature that is being tested and improved/made more robust in master branch.

@saw-jan saw-jan mentioned this pull request Jan 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kobergj kobergj kobergj left review comments

@phil-davis phil-davis phil-davis approved these changes

@wkloucek wkloucek Awaiting requested review from wkloucek wkloucek is a code owner

@micbar micbar Awaiting requested review from micbar micbar is a code owner

@individual-it individual-it Awaiting requested review from individual-it individual-it is a code owner

Assignees

@saw-jan saw-jan

Labels
QA:team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@saw-jan @micbar @phil-davis @kobergj