Skip to content

improve chunked-upload timeout - #2130

Open
alperozturk96 wants to merge 3 commits into
masterfrom
fix/chunked-upload-timeout
Open

improve chunked-upload timeout#2130
alperozturk96 wants to merge 3 commits into
masterfrom
fix/chunked-upload-timeout

Conversation

@alperozturk96

@alperozturk96 alperozturk96 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Tested with 2.5mb, 121.5mb, 245mb, 2GB

Changes

  • Adjust the assemble time
  • Uses Kotlin.Duration instead Int.
  • Add tests for different filesizes and assembled time

Signed-off-by: alperozturk96 <alper_ozturk@proton.me>
Signed-off-by: alperozturk96 <alper_ozturk@proton.me>
Signed-off-by: alperozturk96 <alper_ozturk@proton.me>
@alperozturk96 alperozturk96 changed the title fix(chunked-upload) timeout improve chunked-upload timeout Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

SpotBugs

CategoryBaseNew
Bad practice3131
Correctness2828
Dodgy code1313
Internationalization66
Malicious code vulnerability2626
Multithreaded correctness77
Performance33
Security33
Total117117

@tobiasKaminsky

Copy link
Copy Markdown
Member

Bette to have it in, so that no server times out for us ✔

Mockito.`when`(file.length()).thenReturn(1 * MB)
assertEquals(sut.assembleTimeMin, sut.calculateAssembleTimeout(file))
assertEquals(
ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB / 1000,

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.

expected value should be hardcoded, else it does the same as calc function and thus if there is problem, it will not be seen?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Already like that. I don't get what you mean.

Image

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.

x + y / 1000 is not hardcoded.
I would rather use 3, so that it does not depend on any calculation.

In your initial description you wrote:
min assemble time to 3 min

With "your" test it was wrong/not clear that there is a diff between code and desc.

So having real values, from

  • 3 minimum
  • 13 1Gb
  • 26 2Gb
  • 60 maximum

makes it easier to read, and also reveals any logic error, if computation of it changes?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I got what you mean but having magic numbers in tests makes the test more unclear to me.

These are ASSEMBLE_TIME_BASE, ASSEMBLE_TIME_PER_GB constants. If we change those also magic numbers in the tests needs to be changed.

Those constants part of the production logic. Purpose of the assert is here are we getting expected duration against the calculated assemble time out.

e.g. for 1GB file -> should be ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB and calculateAssembleTimeout must return same value.

ASSEMBLE_TIME_BASE is 1.minutes
ASSEMBLE_TIME_PER_GB is 10.minutes
ASSEMBLE_TIME_MAX is 1.hour

so 11.minutes * 1 = 11 and min of 11.minutes and 1.hour is 11.minutes why we should write -> 11 * 60 * 1000?

@tobiasKaminsky

This comment was marked as outdated.

Sign up for free to 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.

2 participants