Skip to content

Upload part size as S3 parameter instead of constant value - #21317

Merged
rullzer merged 1 commit into
nextcloud:masterfrom
FlorentCoppint:s3_upload_part_size
Jun 9, 2020
Merged

Upload part size as S3 parameter instead of constant value#21317
rullzer merged 1 commit into
nextcloud:masterfrom
FlorentCoppint:s3_upload_part_size

Conversation

@FlorentCoppint

@FlorentCoppintFlorentCoppint commented Jun 8, 2020

Copy link
Copy Markdown
Contributor

Some S3 providers need a custom upload part size (500 MB static value in Nextcloud).

Here is a commit to change this value via S3 configuration, instead of using S3_UPLOAD_PART_SIZE constant.

A new parameter is added for an S3 connection : uploadPartSize

Signed-off-by: Florent florent@coppint.com

@kesselbkesselb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks 👍

protected $timeout;

/** @var int */
protected $upload_part_size;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected$upload_part_size;
protected$uploadPartSize;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use camelCase for variables here.

$this->test = isset($params['test']);
$this->bucket = $params['bucket'];
$this->timeout = !isset($params['timeout']) ? 15 : $params['timeout'];
$this->upload_part_size = !isset($params['upload_part_size']) ? 524288000 : $params['upload_part_size'];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also here for variable and param.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Also here for variable and param.

Hi, are you sure for parameter name ? A lot of parameters are using "underscore style" in Nextcloud

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kesselb

Copy link
Copy Markdown
Contributor

If possible please squash the commits and add the signoff.

As per https://github.com/nextcloud/server/pull/21317/checks?check_run_id=750683865

git commit --amend --signoff
git push --force-with-lease origin s3_upload_part_size

should be enough but that might change with additional commits.

Some S3 providers need a custom upload part size (500 MB static value in Nextcloud).
Here is a commit to change this value via S3 configuration, instead of using S3_UPLOAD_PART_SIZE constant.
A new parameter is added for an S3 connection : uploadPartSize
Signed-off-by: Florent <florent@coppint.com>
@FlorentCoppint

Copy link
Copy Markdown
ContributorAuthor

Do I have something to do about failed Continuous Integration test ?

@kesselb

Copy link
Copy Markdown
Contributor

Do I have something to do about failed Continuous Integration test ?

I don't think so.

@icewind1991icewind1991 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fine by me 👍

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you also update the config.sample.php?

@FlorentCoppint

Copy link
Copy Markdown
ContributorAuthor

Can you also update the config.sample.php?

I can, but there is no S3 example configuration in config.sample.php :(
The feature "S3 as primary storage" is explained in documentation, not in config.sample.php

@rullzer

Copy link
Copy Markdown
Member

Can you also update the config.sample.php?

I can, but there is no S3 example configuration in config.sample.php :(
The feature "S3 as primary storage" is explained in documentation, not in config.sample.php

ah mm goodpoint

@rullzer
rullzer merged commit 8bee8e1 into nextcloud:masterJun 9, 2020
@welcome

welcomeBot commented Jun 9, 2020

Copy link
Copy Markdown

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@FlorentCoppint

Copy link
Copy Markdown
ContributorAuthor

/backport to stable19

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@FlorentCoppint@kesselb@rullzer@icewind1991