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

bugfix/persist fileID over changes of the service instance #1357

Merged
merged 7 commits into from
Feb 12, 2025

Conversation

aaperis
Copy link
Contributor

@aaperis aaperis commented Feb 4, 2025

Related issue(s) and PR(s)

This PR closes a bug first reported by our endusers: #1358.

Description

It provides a fix for the case when for example a client loses connection to the server while uploading a file and when connection is re-established the upload continues through a different s3inbox instance. In such a case, the registered fileID for the file in question would not have been persisted on the new instance, but now it does.

How to test

I found it somewhat difficult to come up with an automated test for this without a heavy refactoring of the code into small batches of separate functions (that would otherwise not serve any purpose) . Any ideas welcome!

I tested it locally by temporarily adding p.fileIds[r.URL.Path] = "" in line 202, building the image and bringing the sda stack up using the make commands, uploading a file to the inbox and seeing that the logs show no errors. Without the fix of the PR the previous procedure will reproduce the error seen on the cluster. Make sure to upload a file from scratch because otherwise the state of the database will render this test obsolete (as it happened when testing my initial fix..).

This should be fixed now. So PR open again.

Note: the failing test is not related to this PR per se.

@aaperis aaperis requested a review from a team February 4, 2025 22:04
@aaperis aaperis self-assigned this Feb 4, 2025
@aaperis aaperis force-pushed the fix/db-connectivity-error branch from 20dbf32 to ddee219 Compare February 4, 2025 22:22
@aaperis aaperis marked this pull request as draft February 6, 2025 16:13
@aaperis aaperis force-pushed the fix/db-connectivity-error branch 3 times, most recently from 5f1f862 to 715b2b3 Compare February 10, 2025 17:33
@aaperis aaperis requested a review from jbygdell February 10, 2025 17:46
@aaperis aaperis marked this pull request as ready for review February 10, 2025 17:46
@aaperis aaperis requested a review from a team February 10, 2025 17:46
@aaperis aaperis force-pushed the fix/db-connectivity-error branch 3 times, most recently from eb5d543 to 518a823 Compare February 11, 2025 09:20
@aaperis aaperis force-pushed the fix/db-connectivity-error branch from 518a823 to b8d3c47 Compare February 11, 2025 09:27
Copy link
Contributor

@nanjiangshu nanjiangshu left a comment

Choose a reason for hiding this comment

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

Works well when I tested according to the description. Just left a minor comment about how the proper error should be returned.

Co-authored-by: Joakim Bygdell <joakim.bygdell@nbis.se>
@aaperis aaperis force-pushed the fix/db-connectivity-error branch from b8d3c47 to 8d0998f Compare February 11, 2025 13:00
@aaperis aaperis requested review from nanjiangshu, jbygdell and a team February 11, 2025 13:05
@aaperis aaperis added the bug Something isn't working label Feb 11, 2025
@aaperis aaperis added this pull request to the merge queue Feb 12, 2025
Merged via the queue into main with commit 6f4ffe2 Feb 12, 2025
27 checks passed
@aaperis aaperis deleted the fix/db-connectivity-error branch February 12, 2025 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants