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: return error when sync context ends #529

Merged
merged 2 commits into from
Nov 30, 2021
Merged

fix: return error when sync context ends #529

merged 2 commits into from
Nov 30, 2021

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Nov 23, 2021

If a sync context ends, return its Context.Err() in the list of sync errors. This informs downstream clients that the sync did not process all entities, even if there were no events in flight (which generate their own errors, as seen below) at the time the context ended. On the user end, this will look like:

11:52:27-0800 esenin $ /tmp/bdeck sync -s /tmp/foo.yaml
creating consumer foo
...
creating upstream foo.8080.svc
^Creceived interrupt , terminating...
Summary:
  Created: 17
  Updated: 0
  Deleted: 0
Error: 5 errors occurred:
	sync context ended, some entities were not synced: context canceled
	while processing event: {Create} upstream bar.8001.svc failed: making HTTP request: Put "http://localhost:8001/upstreams/3acc5ce0-6a3b-40d2-a63c-6ab73d1db221": context canceled
        ...

Fixes #528

If a sync context ends, return its Context.Err() in the list of sync
errors. This informs downstream clients that the sync did not process
all entities.
@rainest rainest requested a review from a team as a code owner November 23, 2021 19:55
@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2021

Codecov Report

Merging #529 (837046a) into main (a01c962) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #529      +/-   ##
==========================================
- Coverage   50.47%   50.46%   -0.01%     
==========================================
  Files          72       72              
  Lines        7870     7871       +1     
==========================================
  Hits         3972     3972              
- Misses       3549     3550       +1     
  Partials      349      349              
Impacted Files Coverage Δ
diff/diff.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a01c962...837046a. Read the comment docs.

hbagdi
hbagdi previously approved these changes Nov 29, 2021
Copy link
Member

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

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

I'm not certain if this will indeed solve the issue or not but is minor enough to give it a try.

diff/diff.go Outdated Show resolved Hide resolved
Co-authored-by: Harry <harrybagdi@gmail.com>
@rainest rainest enabled auto-merge (squash) November 29, 2021 17:54
@rainest rainest requested a review from hbagdi November 29, 2021 18:44
@rainest rainest merged commit 1b8f774 into main Nov 30, 2021
@rainest rainest deleted the fix/report-timeout branch November 30, 2021 16:54
@rainest rainest mentioned this pull request Dec 2, 2021
AntoineJac pushed a commit that referenced this pull request Jan 23, 2024
If a sync context ends, return its Context.Err() in the list of sync
errors. This informs downstream clients that the sync did not process
all entities.

Co-authored-by: Harry <harrybagdi@gmail.com>
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.

Solver reports success when terminated early by context expiration
3 participants