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

Refactor scheduler/polling out of scheduled-feed/main.go #121

Merged
merged 1 commit into from
May 20, 2021

Conversation

Qinusty
Copy link
Contributor

@Qinusty Qinusty commented May 19, 2021

scheduled-feed/main.go was becoming quite cluttered as functionality has been added, this PR remedies this by shifting many of the responsibilities to the scheduler with additional structs being introduced to handle the polling of feed collections.

@Qinusty Qinusty force-pushed the qinusty/refactor-scheduling branch from de9313c to 838ba21 Compare May 20, 2021 09:39
@Qinusty Qinusty changed the title WIP: Refactor scheduler/polling out of scheduled-feed/main.go Refactor scheduler/polling out of scheduled-feed/main.go May 20, 2021
@Qinusty Qinusty requested a review from tom--pollard May 20, 2021 09:41
@tom--pollard
Copy link
Contributor

Other than my previous comments, this looks a lot cleaner! It also opens up a smoother path to introducing per feed polling rates, as mentioned in #85

@tom--pollard
Copy link
Contributor

tom--pollard commented May 20, 2021

Running the binary locally, and triggering via a remote curl, at the end of the logs I get:

time="2021-05-20T11:17:11Z" level=info msg="Packages successfully processed" feed=pypi num_processed=0
time="2021-05-20T11:17:11Z" level=info msg="Packages successfully processed" feed=goproxy num_processed=0
time="2021-05-20T11:17:11Z" level=info msg="Packages successfully processed" feed=packagist num_processed=0
time="2021-05-20T11:17:12Z" level=info msg="Packages successfully processed" feed=crates num_processed=0
time="2021-05-20T11:17:13Z" level=info msg="Packages successfully processed" feed=nuget num_processed=0
time="2021-05-20T11:17:13Z" level=info msg="Packages successfully processed" feed=npm num_processed=0
time="2021-05-20T11:17:13Z" level=info msg="Packages successfully processed" feed=rubygems num_processed=0
time="2021-05-20T11:17:13Z" level=info msg="0 packages processed"

I've noticed before these changes that the num_processed logging can sometimes be odd, but we should probably look to address this in the PR.

At a glance, I think it's due to WithField("num_processed", len(result.packages)) expecting an interface, not an int.

@Qinusty
Copy link
Contributor Author

Qinusty commented May 20, 2021

I've noticed before these changes that the num_processed logging can sometimes be odd, but we should probably look to address this in the PR.

num_processed=0 is due to no packages being processed within the cutoff window I believe. I'm not witnessing this with longer pollRate values.

@Qinusty Qinusty force-pushed the qinusty/refactor-scheduling branch from 838ba21 to 9a11ef6 Compare May 20, 2021 12:13
@tom--pollard
Copy link
Contributor

tom--pollard commented May 20, 2021

I've noticed before these changes that the num_processed logging can sometimes be odd, but we should probably look to address this in the PR.

num_processed=0 is due to no packages being processed within the cutoff window I believe. I'm not witnessing this with longer pollRate values.

I'm not sure that's the case, on a clean execution with the default time, I get multiple sending package upstream logs, and a return of n packages processed on the http request. This log happens at polltime, and it does seem to correctly report per feed, but then at the end of the logs again as zero... which would suggest something is triggering another poll immediately?

  • I can't now replicate this with curl, but I can with a web browser which is seemingly making 2 requests, as such the second requests gets zero hits due to the preceding cutoff being applied.

@Qinusty Qinusty force-pushed the qinusty/refactor-scheduling branch from 9a11ef6 to 9d0b1f3 Compare May 20, 2021 12:37
Copy link
Contributor

@tom--pollard tom--pollard left a comment

Choose a reason for hiding this comment

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

LGTM!

@Qinusty Qinusty merged commit 0628dda into ossf:main May 20, 2021
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.

2 participants