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

feeds/: Allow for per package/feed error handling when polling #123

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

tom--pollard
Copy link
Contributor

@tom--pollard tom--pollard commented May 21, 2021

Resolves #107

feeds/: Allow for per package/feed error handling when polling

When errors occur but package data can still be processed then
the system should continue where possible. This is particularly
beneficial in feeds where data is fetched on a per package basis,
as a singular failure shouldn't immediately lead to data loss
for the whole session. `pollAndPublish()` error handling and
subsequent callers have been modified to handle this.

This also removes `errUnpublished` as a 'hard' error when polling
npm for critical packages as such it is only logged.

Follow ups in the form of #101 and more tests/granular handling in nuget & packagist should also be undertaken

@tom--pollard tom--pollard force-pushed the handle_per_package_err branch 4 times, most recently from 6d2d85f to c23cde4 Compare May 27, 2021 11:02
@tom--pollard tom--pollard changed the title WIP: feeds/pypi.go: Only fail if all packages fail to be polled for feeds/pypi.go: Only fail if all packages fail to be polled for May 27, 2021
@tom--pollard
Copy link
Contributor Author

If it's deemed necessary I can look to add this behaviour for all feeds in a single PR

feeds/feed.go Outdated Show resolved Hide resolved
feeds/pypi/pypi.go Show resolved Hide resolved
feeds/pypi/pypi.go Outdated Show resolved Hide resolved
feeds/feed.go Outdated Show resolved Hide resolved
@tom--pollard tom--pollard changed the title feeds/pypi.go: Only fail if all packages fail to be polled for WIP: Allow for per package error handling when polling Jun 7, 2021
@tom--pollard tom--pollard changed the title WIP: Allow for per package error handling when polling feeds/: Allow for per package/feed error handling when polling Jun 8, 2021
@tom--pollard tom--pollard force-pushed the handle_per_package_err branch 5 times, most recently from 8c3cefe to 28a10ec Compare June 8, 2021 14:03
@tom--pollard tom--pollard requested a review from Qinusty June 8, 2021 14:07
@tom--pollard
Copy link
Contributor Author

@Qinusty In terms of beyond general review, I'm particularly interested in ensuring nuget/packagist are ok as it's my first time working with those feed implementations. I'm also happy to split any new tests into a separate commit once reviewed.

Copy link
Contributor

@Qinusty Qinusty left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, if we can fix this outstanding issue it looks good to go

feeds/scheduler/feed_group.go Outdated Show resolved Hide resolved
@tom--pollard tom--pollard force-pushed the handle_per_package_err branch 2 times, most recently from e364346 to b959d60 Compare June 9, 2021 17:00
feeds/scheduler/feed_group.go Outdated Show resolved Hide resolved
feeds/scheduler/feed_group.go Outdated Show resolved Hide resolved
@tom--pollard tom--pollard force-pushed the handle_per_package_err branch 2 times, most recently from e42e43b to d79dc2a Compare June 10, 2021 13:17
When errors occur but package data can still be processed then
the system should continue where possible. This is particularly
beneficial in feeds where data is fetched on a per package basis,
as a singular failure shouldn't immediately lead to data loss
for the whole session. `pollAndPublish()` error handling and
subsequent callers have been modified to handle this.

This also removes `errUnpublished` as a 'hard' error when polling
npm for critical packages as such it is only logged.
Copy link
Contributor

@Qinusty Qinusty left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

Handle errors more granularly when polling for specific packages
2 participants