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

core(runner): abstract gather phase out of runner #11623

Merged
merged 2 commits into from
Nov 4, 2020

Conversation

patrickhulce
Copy link
Collaborator

Summary
The runner related changes for FR phase 1. See #11622 for how it will eventually come together. Goal here is to eliminate the reliance of the Runner on an explicit connection and abstract away the gathering.

Related Issues/PRs
ref #11313

@patrickhulce patrickhulce requested a review from a team as a code owner November 2, 2020 18:10
@patrickhulce patrickhulce requested review from adamraine and removed request for a team November 2, 2020 18:10
@google-cla google-cla bot added the cla: yes label Nov 2, 2020
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

One thing we've talked about at various times in the past has been splitting out gathering and auditing into even more distinct steps, basically turning gather-runner into the -G runner and runner into the -A runner (an audit-runner, if you will :).

That's another option here, basically passing in artifacts (procured however) into runner, which would avoid the inversion of control of the callback. I have a super ancient branch that explored this a bit, adding an explicit audit-runner file while runner remains as the orchestrator of the two, but I don't think there's any reason to necessarily go that way with runner...instead code calling gather() and then audit() could be kicked back up to index.

That does involve more code and test rearranging, though, than this very succinct change.

@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Nov 2, 2020

I agree with that overall direction and I think we'll get there when all is said and done at the end of this (the changes for timespan will force that issue a bit more than the phase 1 snapshot mode does where some key concepts violate the existing runner structure). I think the main part of this decision comes down to how would others on the team prefer that FR affect other areas? Would early "unnecessary" refactors without understanding the need for them yet be preferable to finding out too late that some big refactor might be necessary later?

This PR reflects my preference for deferral in case things change, but if, as a whole, we'd rather frontload some of those changes before FR functionality arrives that also seems OK.

That does involve more code and test rearranging, though, than this very succinct change.

My goal was to avoid that for the purposes of early FR unblocking :)

@brendankenny
Copy link
Member

This PR reflects my preference for deferral in case things change, but if, as a whole, we'd rather frontload some of those changes before FR functionality arrives that also seems OK.

That does involve more code and test rearranging, though, than this very succinct change.

My goal was to avoid that for the purposes of early FR unblocking :)

Well, if you're open to the general design and willing to review, one option is to unblock FR with this PR, then I could make the change which would allow frontloading without slowing down the other work you have in the pipeline (assuming everything is compatible, which is seems like it would be).

@patrickhulce
Copy link
Collaborator Author

Sounds good 👍

Copy link
Member

@adamraine adamraine 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants