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

Improve CTQ notifier #152

Closed
3 of 4 tasks
chrisklus opened this issue Jul 6, 2022 · 8 comments
Closed
3 of 4 tasks

Improve CTQ notifier #152

chrisklus opened this issue Jul 6, 2022 · 8 comments
Assignees

Comments

@chrisklus
Copy link
Contributor

chrisklus commented Jul 6, 2022

This issue is for the remaining work to do for CTQ notifier:

@chrisklus
Copy link
Contributor Author

@samreid reported in phetsims/chipper#1269 (comment) an idea for a better way to notify CTQ that a set of pushes is complete! This would address the third checkbox in the list above.

@samreid
Copy link
Member

samreid commented Jul 9, 2022

The same idea could be leveraged for the full CT like so:

  • The push script notifies that it is about to push certain repos. fetch('http://bayes.colorado.edu/ct/prepush?repos=sim1,sim2,common1)
  • The push script pushes the repos
  • The push script notifies that it has pushed the repos. fetch('http://bayes.colorado.edu/ct/postpush?repos=sim1,sim2,common1)

Then CT and CTQ can identify whether they are in a consistent state if they aren't waiting for any prepushes to finish. Or maybe we would just use a random token like start=n2th1nth23n12th3n and end=n2th1nth23n12th3n. CT could give up if it doesn't receive the end for a start within N minutes.

@pixelzoom
Copy link
Contributor

In Slack#continuous-testing, I suggested something else:

It seems like many (most?) of the CTQ false positives (aka 🦤) occur when pushing to multiple repos. I don’t know how CT knows when to pull repos. But I suspect it notices that a repo has changed, then pulls only that repo. Could this problem be resolved (or reduced in frequency) by introducing a slight delay after CT notices any repo change, then doing a pull-all?

@pixelzoom
Copy link
Contributor

In Slack#continuous-testing, @zepumph said:

The logic is here, it would be simple enough to instead wait 20 seconds before running it.

// TODO: don't be synchronous here
const staleRepos = [];
winston.info( 'QuickServer: checking stale' );
await Promise.all( reposToCheck.map( async repo => {
if ( await isStale( repo ) ) {
staleRepos.push( repo );
winston.info( `QuickServer: ${repo} stale` );
}
} ) );
if ( staleRepos.length || forceTests ) {

@zepumph
Copy link
Member

zepumph commented Nov 2, 2022

@chrisklus, 1de7fdf was causing a bunch of problems because it was kicking off infinite numbers of setTimeout calls and running out of memory on the javascript heap. We need to synchronously pause execution of the while loop if we find a staleRepo. Let's try this one.

@zepumph
Copy link
Member

zepumph commented Mar 14, 2023

Working on this today as part of #166

@chrisklus
Copy link
Contributor Author

@zepumph do you think we can close this issue? Or wait another month or so to see if item 3 in #152 (comment) needs more work?

@chrisklus chrisklus assigned zepumph and unassigned chrisklus Apr 28, 2023
@zepumph
Copy link
Member

zepumph commented Apr 28, 2023

I was just thinking that! I do not think any more is needed here. Thanks

@zepumph zepumph closed this as completed Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants