presto icon indicating copy to clipboard operation
presto copied to clipboard

Add concurrency constraints

Open jsoref opened this issue 2 years ago • 4 comments

Test plan - Create a branch, create a PR, push another commit to the branch, verify that the previous workflows are mostly killed.

https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#concurrency

== NO RELEASE NOTE ==

jsoref avatar Mar 03 '22 08:03 jsoref

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

stale[bot] avatar Sep 21 '22 09:09 stale[bot]

Obviously I'd prefer to have someone review my change...

jsoref avatar Sep 21 '22 13:09 jsoref

@jsoref could you please rebase this PR?

rohanpednekar avatar Sep 22 '22 16:09 rohanpednekar

@rohanpednekar done

jsoref avatar Sep 22 '22 18:09 jsoref

Will take a look at it over the weekend.

pratyakshsharma avatar Sep 30 '22 09:09 pratyakshsharma

@pratyakshsharma ?

ethanyzhang avatar Oct 13 '22 17:10 ethanyzhang

Going over the github doc for workflow and concurrency as mentioned in the description of this PR. Will review post going through them.

pratyakshsharma avatar Oct 27 '22 13:10 pratyakshsharma

@pratyakshsharma: this should do what you need... (again you'll want to test to confirm)

jsoref avatar Oct 28 '22 19:10 jsoref

I tested and the changes look good. Here is a sample run that I triggered - https://github.com/prestodb/presto/actions/runs/3359687457.

You might want to check why all the checks got cancelled for the latest commit though.

pratyakshsharma avatar Oct 31 '22 08:10 pratyakshsharma

Hmm, strange. My guess is that GitHub got confused and used the wrong version of the workflow to test. (I'd have to spend a lot of time poking and then file a bug).

Here's a PR I've created into a branch w/ the same name and sha: https://github.com/check-spelling/presto/actions?query=branch%3Ablank+event%3Apull_request

It'll take a while to clear as the workflows here use a lot of runners and I only have 20 slots.

jsoref avatar Oct 31 '22 13:10 jsoref

So, in https://github.com/check-spelling/presto/actions?query=branch%3Ablank+event%3Apull_request, everything looked more or less ok: image https://github.com/check-spelling/presto/actions/runs/3361686022/jobs/5572411876 failed with a test failure:

Error:  Failures: 
Error:    TestTimeZoneUtils.testNamedZones:77 » IllegalArgument The datetime zone id 'Europe/Kyiv' is not recognised
[INFO] 
Error:  Tests run: 7416, Failures: 1, Errors: 0, Skipped: 1

https://github.com/check-spelling/presto/actions/runs/3361686030 has: product-tests-specific-environment1 The operation was canceled. which sounded suspiciously like a problem, but in fact, it's just the job hitting the 1 hour time limit: image https://github.com/check-spelling/presto/actions/runs/3361686030/workflow#L15-L17

  product-tests-specific-environment1:
    runs-on: ubuntu-latest
    timeout-minutes: 60

jsoref avatar Oct 31 '22 16:10 jsoref

Here's a run 7 which was mostly canceled by a run 8: image

I say mostly, because it looks like some new workflows were added between when this PR was originally written and now.

Fwiw, there's no meaningful distinction between a ❕ and ❌, it's just an implementation quirk of how GitHub dispatches the cancelations and reports on them. By quirk, I mean "I consider this to be a bug, but I haven't managed to convince GitHub to fix it".

jsoref avatar Oct 31 '22 16:10 jsoref

Apologies for the long delay in getting this through. I tested the latest changes and found 2 different types of cancellations like you already mentioned. Anyhow your changes seem to be working and they look good to me.

pratyakshsharma avatar Jan 03 '23 08:01 pratyakshsharma

Screenshot 2023-01-03 at 2 17 46 PM

pratyakshsharma avatar Jan 03 '23 08:01 pratyakshsharma

Screenshot 2023-01-03 at 2 17 32 PM

pratyakshsharma avatar Jan 03 '23 08:01 pratyakshsharma

@prestodb/committers This PR has been lingering for some time now. I have gone through the changes and tested them, and they look good to me.

Let us get this merged.

pratyakshsharma avatar Jan 03 '23 08:01 pratyakshsharma

@rschlussel done

jsoref avatar Jan 03 '23 16:01 jsoref

The first canceled here is a test that hit the 1 hour mark (which is presumably a timeout) and not related to the changes in this PR.

jsoref avatar Jan 03 '23 17:01 jsoref