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

Retry results #730

Merged
merged 1 commit into from
Jun 5, 2019
Merged

Retry results #730

merged 1 commit into from
Jun 5, 2019

Conversation

johnSchnake
Copy link
Contributor

Will be rebased/tweaked after #729 merges which simplifies this update/testing since it puts all the logic in one place.

What this PR does / why we need it:
In the case where a connection gets reset or other
processing errors occur, the client is already
coded to automatically retry submission of the
results.

However, the server was marking the results as 'seen'
and would not reprocess them.

This change allows reprocessing results only if there
were errors processing before.

Which issue(s) this PR fixes
Fixes #728

Special notes for your reviewer:
FWIW, with the centralized code I think some other tests could be fixed up and made to read a bit better but it's not really worth it to go and rework older tests.

Release note:

Fixed a bug which led to partial results for plugins when there were connection resets and other networking issues.

@johnSchnake johnSchnake requested a review from stevesloka May 25, 2019 06:39
@codecov-io
Copy link

codecov-io commented May 25, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@98d7d32). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #730   +/-   ##
=========================================
  Coverage          ?   40.47%           
=========================================
  Files             ?       69           
  Lines             ?     3943           
  Branches          ?        0           
=========================================
  Hits              ?     1596           
  Misses            ?     2248           
  Partials          ?       99
Impacted Files Coverage Δ
pkg/plugin/aggregation/aggregator.go 75.86% <100%> (ø)

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 98d7d32...338fe75. Read the comment docs.

pkg/plugin/aggregation/aggregator.go Outdated Show resolved Hide resolved
pkg/plugin/aggregation/aggregator.go Outdated Show resolved Hide resolved
In the case where a connection gets reset or other
processing errors occur, the client is already
coded to automatically retry submission of the
results.

However, the server was marking the results as 'seen'
and would not reprocess them.

This change allows reprocessing results only if there
were errors processing before.

Fixes #728

Signed-off-by: John Schnake <jschnake@vmware.com>
Copy link
Contributor

@stevesloka stevesloka left a comment

Choose a reason for hiding this comment

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

/lgtm

@stevesloka stevesloka merged commit 4d4ff7a into vmware-tanzu:master Jun 5, 2019
@johnSchnake johnSchnake deleted the retryResults branch June 5, 2019 14:57
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.

Aggregator doesnt properly handle errors when getting results
3 participants