Fix 2 issues with the upload.s3 component#40
Merged
Conversation
tommyblue
force-pushed
the
fix_2_upload_issues
branch
2 times, most recently
from
70305f7 to
1001869
Compare
tommyblue
force-pushed
the
fix_2_upload_issues
branch
2 times, most recently
from
85d051c to
a736446
Compare
1) #27 introduced an issue: the upload returned too soon and the last file in the logs path wasn't uploaded 2) we found an existing issue that caused the last file in the staging path not to be uploaded. Now the last upload is performed when the upload channel is closed
tommyblue
force-pushed
the
fix_2_upload_issues
branch
from
a736446 to
1c33a3e
Compare
arl
approved these changes
Sep 18, 2020
Collaborator
arl
left a comment
arl
left a comment
There was a problem hiding this comment.
LGTM.
At one point we should add some tests very similar to TestS3Upload but use the actual topology rather than emulating it, in order to be closer to the actual production usage, whether baker ends by itself after input exhaustion or because of a CTRL-C
Comment on lines
+126
to
+128
| stagingDir, rmStagingDir := testutil.TempDir(t) | ||
| defer rmStagingDir() | ||
|
|
Collaborator
There was a problem hiding this comment.
Given that we're now using go1.15, we can directly use https://golang.org/pkg/testing/#T.TempDir
Collaborator
|
@tommyblue you can merge this now if we want to add later the tests I was talking about. In the case we're not doing this in this PR, it would be great to create a ticket for that (in case it hasn't already been created), thanks. |
tommyblue
commented
Sep 18, 2020
| } | ||
| log.WithFields(log.Fields{"filepath": sourceFilePath}).WithError(err).Error("couldn't move") | ||
| } | ||
| case <-u.quit: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
❓ What
upload: s3: add FailOnError config #27 introduced an issue: the
upload returned too soon and the last file in the logs path wasn't
uploaded
we found an existing issue that caused an optional last file in the staging
path not to be uploaded. Changing the topology stop flow, now the uploader waits the
upchto be closed, thus correctly running the last upload after the lastmove(if any)🔨 How to test
To test this change I used 2 topologies and 3 use cases:
✅ Checklists
make gofmt-writebeen run on the code?make govetbeen run on the code? Has the code been fixed accordingly to the output?