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

fix(attachment): convoy separation #7449

Merged
merged 4 commits into from
Jun 27, 2024

Conversation

KenLSM
Copy link
Contributor

@KenLSM KenLSM commented Jun 27, 2024

Problem

Closes FRM-1757

Admins are not able to receive all the download responses when clicking download with CSV.

Most modern browser blocks sequential downloads that run in quick successions.

Symptoms

  • results are mainly without attachments
  • responses > 10

Solution

Implementation of convoy separation.

Downloads are grouped into convoys where the front of the convoy must be x ms away from the next convoy.

[ o . o . . o o o ] . . [ o o o . . o . o] [ o o o o ]
  ^                       ^                  ^
  1st convoy              2nd convoy         3rd convoy                  

CONVOY_SIZE = 5
The time between each convoy should X amount of distance away to avoid a flood
3rd Convoy is too close and thus will be forced to wait

[ o . o . . o o o ] . . [ o o o . . o . o] . . . . [ o o o o ]
  ^                       ^                          ^
  1st convoy              2nd convoy                 3rd convoy

Additional Context

Total responses = 44
Experiments:

Control Group

  • no delay -> get 20 zips

Batch delay

  • 1000ms of delay every 7 files -> get all 44 zips (increased completion time of 6 seconds)
  • 200ms of delay every 7 files -> get 22 zips

Convoy delay (CONVOY_SIZE = 7)

  • 1000ms between first of next convoy, -> get 39 zips

Convoy delay (CONVOY_SIZE = 5)

  • 1000ms between first of next convoy, -> get all 44 zips

Breaking Changes

  • No - this PR is backwards compatible

Tests

Regression on large submissions >10 crossing two convoys

  • Create 40 submissions with 20 attachments
  • Ensure that Download CSV (without attachment) returns 1 .csv file
  • Ensure that Download CSV with attachment returns 40 .zip and 1 .csv file

Regression on small submissions < 10

  • Create 7 submissions with 3 attachments only 1 convoy
  • Ensure that Download CSV (without attachment) returns 1 .csv file
  • Ensure that Download CSV with attachment returns 7 .zip and 1 .csv file

Copy link
Contributor

@g-tejas g-tejas 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 algo looks good to me

@KenLSM KenLSM changed the base branch from release-al2 to release-v6.130.1 June 27, 2024 10:49
@KenLSM KenLSM changed the title hotfix(attachment): convoy separation fix(attachment): convoy separation Jun 27, 2024
@KenLSM KenLSM merged commit 6a0823f into release-v6.130.1 Jun 27, 2024
7 of 8 checks passed
@KenLSM KenLSM deleted the hotfix/attachment-convoy-separation branch June 27, 2024 13:44
KenLSM added a commit that referenced this pull request Jun 28, 2024
fix(attachment): convoy separation (#7449)
@KenLSM KenLSM mentioned this pull request Jun 30, 2024
30 tasks
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.

None yet

2 participants