Import `os` in executor_loader by jedcunningham · Pull Request #44927 · apache/airflow

jedcunningham · 2024-12-14T00:00:04Z

That import was removed in #44839, but #44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

o-nikolas · 2024-12-16T17:56:57Z

Thanks @jedcunningham
I'm not sure how that happened or wasn't detected. Seems like a test should have failed but didn't? Are we missing coverage?

jedcunningham · 2024-12-16T19:41:22Z

No worries, this is just a standard "branch wasn't up to date with main, and tests run from the branch" scenario. Nothing missing really, just every so often we will have to fix these types of things.

potiuk · 2024-12-17T13:18:29Z

Cross-merging PRs. yeah.

And actually yes we could do something about it - that could go away if we had the possibility of using "Merge Queue" functionality: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue

The merge queue functionality works in the way, that instead of merging, committer adds PR to merge queue, and they are run in sequence - automatically rebasing them after the previously merged PR is merged - and only actually merging the PR when that "queued" PR had succeded again after rebase.

So far it was blocked as INFRA tooling is unable to retrieve "merger identity" for necessary ICLA "audit logs" when merge queue is used. But @davidarthur from Kafka team run a sandbox environment https://issues.apache.org/jira/browse/INFRA-25932 where he had shown how it is possible - so now it hangs a bit on INFRA making a decision whether to invest in it, implement missing piece and enable it.

If people here would like to comment on ths INFRA ticket and encourage it, having the possibility of using Merge Queues could help us to avoid this kind of issues, without impacting the velocity of merges.

o-nikolas · 2024-12-17T17:49:24Z

That sounds like an awesome mechanism @potiuk I added a +1 to the ticket (I didn't want to pollute the comment stream where details are being hashed out).

ashb · 2024-12-17T17:53:37Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

potiuk · 2024-12-17T19:41:53Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

Not when we go back and have ARC enabled, our tests elapsed time will go back to 20 minutes, and we already have done pretty damn good job (and continue doing so) on battling flaky tests. With our team effort we already have sometimes days without flaky tests, and yes while I would not want to use merge queues two months ago, I think we are pretty much ready to get it and get it much more stable (especially after we bring back 16 processor machines in our S3 to complete our tests 4 and sometimes even 8 times faster than the current builds. CC: @hussein-awala :D

potiuk · 2024-12-17T19:43:27Z

PLus merge queue will STILL use the selective checks optimizations - that is not going to change - so most of the PRS will be done in 10 minutes even without ARC (and maybe less than 4-5 minutes with ARC).

o-nikolas · 2024-12-17T20:55:05Z

I also think consistency is very important and worth paying a time cost for. Something could have been dropped that was harder to detect except in narrow runtime circumstances.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

jedcunningham · 2024-12-14T00:00:04Z

That import was removed in #44839, but #44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

o-nikolas · 2024-12-16T17:56:57Z

Thanks @jedcunningham
I'm not sure how that happened or wasn't detected. Seems like a test should have failed but didn't? Are we missing coverage?

jedcunningham · 2024-12-16T19:41:22Z

No worries, this is just a standard "branch wasn't up to date with main, and tests run from the branch" scenario. Nothing missing really, just every so often we will have to fix these types of things.

potiuk · 2024-12-17T13:18:29Z

Cross-merging PRs. yeah.

And actually yes we could do something about it - that could go away if we had the possibility of using "Merge Queue" functionality: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue

The merge queue functionality works in the way, that instead of merging, committer adds PR to merge queue, and they are run in sequence - automatically rebasing them after the previously merged PR is merged - and only actually merging the PR when that "queued" PR had succeded again after rebase.

So far it was blocked as INFRA tooling is unable to retrieve "merger identity" for necessary ICLA "audit logs" when merge queue is used. But @davidarthur from Kafka team run a sandbox environment https://issues.apache.org/jira/browse/INFRA-25932 where he had shown how it is possible - so now it hangs a bit on INFRA making a decision whether to invest in it, implement missing piece and enable it.

If people here would like to comment on ths INFRA ticket and encourage it, having the possibility of using Merge Queues could help us to avoid this kind of issues, without impacting the velocity of merges.

o-nikolas · 2024-12-17T17:49:24Z

That sounds like an awesome mechanism @potiuk I added a +1 to the ticket (I didn't want to pollute the comment stream where details are being hashed out).

ashb · 2024-12-17T17:53:37Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

potiuk · 2024-12-17T19:41:53Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

Not when we go back and have ARC enabled, our tests elapsed time will go back to 20 minutes, and we already have done pretty damn good job (and continue doing so) on battling flaky tests. With our team effort we already have sometimes days without flaky tests, and yes while I would not want to use merge queues two months ago, I think we are pretty much ready to get it and get it much more stable (especially after we bring back 16 processor machines in our S3 to complete our tests 4 and sometimes even 8 times faster than the current builds. CC: @hussein-awala :D

potiuk · 2024-12-17T19:43:27Z

PLus merge queue will STILL use the selective checks optimizations - that is not going to change - so most of the PRS will be done in 10 minutes even without ARC (and maybe less than 4-5 minutes with ARC).

o-nikolas · 2024-12-17T20:55:05Z

I also think consistency is very important and worth paying a time cost for. Something could have been dropped that was harder to detect except in narrow runtime circumstances.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

jedcunningham · 2024-12-14T00:00:04Z

That import was removed in #44839, but #44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

o-nikolas · 2024-12-16T17:56:57Z

Thanks @jedcunningham
I'm not sure how that happened or wasn't detected. Seems like a test should have failed but didn't? Are we missing coverage?

jedcunningham · 2024-12-16T19:41:22Z

No worries, this is just a standard "branch wasn't up to date with main, and tests run from the branch" scenario. Nothing missing really, just every so often we will have to fix these types of things.

potiuk · 2024-12-17T13:18:29Z

Cross-merging PRs. yeah.

And actually yes we could do something about it - that could go away if we had the possibility of using "Merge Queue" functionality: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue

The merge queue functionality works in the way, that instead of merging, committer adds PR to merge queue, and they are run in sequence - automatically rebasing them after the previously merged PR is merged - and only actually merging the PR when that "queued" PR had succeded again after rebase.

So far it was blocked as INFRA tooling is unable to retrieve "merger identity" for necessary ICLA "audit logs" when merge queue is used. But @davidarthur from Kafka team run a sandbox environment https://issues.apache.org/jira/browse/INFRA-25932 where he had shown how it is possible - so now it hangs a bit on INFRA making a decision whether to invest in it, implement missing piece and enable it.

If people here would like to comment on ths INFRA ticket and encourage it, having the possibility of using Merge Queues could help us to avoid this kind of issues, without impacting the velocity of merges.

o-nikolas · 2024-12-17T17:49:24Z

That sounds like an awesome mechanism @potiuk I added a +1 to the ticket (I didn't want to pollute the comment stream where details are being hashed out).

ashb · 2024-12-17T17:53:37Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

potiuk · 2024-12-17T19:41:53Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

Not when we go back and have ARC enabled, our tests elapsed time will go back to 20 minutes, and we already have done pretty damn good job (and continue doing so) on battling flaky tests. With our team effort we already have sometimes days without flaky tests, and yes while I would not want to use merge queues two months ago, I think we are pretty much ready to get it and get it much more stable (especially after we bring back 16 processor machines in our S3 to complete our tests 4 and sometimes even 8 times faster than the current builds. CC: @hussein-awala :D

potiuk · 2024-12-17T19:43:27Z

PLus merge queue will STILL use the selective checks optimizations - that is not going to change - so most of the PRS will be done in 10 minutes even without ARC (and maybe less than 4-5 minutes with ARC).

o-nikolas · 2024-12-17T20:55:05Z

I also think consistency is very important and worth paying a time cost for. Something could have been dropped that was harder to detect except in narrow runtime circumstances.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

jedcunningham · 2024-12-14T00:00:04Z

That import was removed in #44839, but #44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

o-nikolas · 2024-12-16T17:56:57Z

Thanks @jedcunningham
I'm not sure how that happened or wasn't detected. Seems like a test should have failed but didn't? Are we missing coverage?

jedcunningham · 2024-12-16T19:41:22Z

No worries, this is just a standard "branch wasn't up to date with main, and tests run from the branch" scenario. Nothing missing really, just every so often we will have to fix these types of things.

potiuk · 2024-12-17T13:18:29Z

Cross-merging PRs. yeah.

And actually yes we could do something about it - that could go away if we had the possibility of using "Merge Queue" functionality: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue

The merge queue functionality works in the way, that instead of merging, committer adds PR to merge queue, and they are run in sequence - automatically rebasing them after the previously merged PR is merged - and only actually merging the PR when that "queued" PR had succeded again after rebase.

So far it was blocked as INFRA tooling is unable to retrieve "merger identity" for necessary ICLA "audit logs" when merge queue is used. But @davidarthur from Kafka team run a sandbox environment https://issues.apache.org/jira/browse/INFRA-25932 where he had shown how it is possible - so now it hangs a bit on INFRA making a decision whether to invest in it, implement missing piece and enable it.

If people here would like to comment on ths INFRA ticket and encourage it, having the possibility of using Merge Queues could help us to avoid this kind of issues, without impacting the velocity of merges.

o-nikolas · 2024-12-17T17:49:24Z

That sounds like an awesome mechanism @potiuk I added a +1 to the ticket (I didn't want to pollute the comment stream where details are being hashed out).

ashb · 2024-12-17T17:53:37Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

potiuk · 2024-12-17T19:41:53Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

Not when we go back and have ARC enabled, our tests elapsed time will go back to 20 minutes, and we already have done pretty damn good job (and continue doing so) on battling flaky tests. With our team effort we already have sometimes days without flaky tests, and yes while I would not want to use merge queues two months ago, I think we are pretty much ready to get it and get it much more stable (especially after we bring back 16 processor machines in our S3 to complete our tests 4 and sometimes even 8 times faster than the current builds. CC: @hussein-awala :D

potiuk · 2024-12-17T19:43:27Z

PLus merge queue will STILL use the selective checks optimizations - that is not going to change - so most of the PRS will be done in 10 minutes even without ARC (and maybe less than 4-5 minutes with ARC).

o-nikolas · 2024-12-17T20:55:05Z

I also think consistency is very important and worth paying a time cost for. Something could have been dropped that was harder to detect except in narrow runtime circumstances.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

jedcunningham · 2024-12-14T00:00:04Z

That import was removed in #44839, but #44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

o-nikolas · 2024-12-16T17:56:57Z

Thanks @jedcunningham
I'm not sure how that happened or wasn't detected. Seems like a test should have failed but didn't? Are we missing coverage?

jedcunningham · 2024-12-16T19:41:22Z

No worries, this is just a standard "branch wasn't up to date with main, and tests run from the branch" scenario. Nothing missing really, just every so often we will have to fix these types of things.

potiuk · 2024-12-17T13:18:29Z

Cross-merging PRs. yeah.

And actually yes we could do something about it - that could go away if we had the possibility of using "Merge Queue" functionality: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue

The merge queue functionality works in the way, that instead of merging, committer adds PR to merge queue, and they are run in sequence - automatically rebasing them after the previously merged PR is merged - and only actually merging the PR when that "queued" PR had succeded again after rebase.

So far it was blocked as INFRA tooling is unable to retrieve "merger identity" for necessary ICLA "audit logs" when merge queue is used. But @davidarthur from Kafka team run a sandbox environment https://issues.apache.org/jira/browse/INFRA-25932 where he had shown how it is possible - so now it hangs a bit on INFRA making a decision whether to invest in it, implement missing piece and enable it.

If people here would like to comment on ths INFRA ticket and encourage it, having the possibility of using Merge Queues could help us to avoid this kind of issues, without impacting the velocity of merges.

o-nikolas · 2024-12-17T17:49:24Z

That sounds like an awesome mechanism @potiuk I added a +1 to the ticket (I didn't want to pollute the comment stream where details are being hashed out).

ashb · 2024-12-17T17:53:37Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

potiuk · 2024-12-17T19:41:53Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

Not when we go back and have ARC enabled, our tests elapsed time will go back to 20 minutes, and we already have done pretty damn good job (and continue doing so) on battling flaky tests. With our team effort we already have sometimes days without flaky tests, and yes while I would not want to use merge queues two months ago, I think we are pretty much ready to get it and get it much more stable (especially after we bring back 16 processor machines in our S3 to complete our tests 4 and sometimes even 8 times faster than the current builds. CC: @hussein-awala :D

potiuk · 2024-12-17T19:43:27Z

PLus merge queue will STILL use the selective checks optimizations - that is not going to change - so most of the PRS will be done in 10 minutes even without ARC (and maybe less than 4-5 minutes with ARC).

o-nikolas · 2024-12-17T20:55:05Z

I also think consistency is very important and worth paying a time cost for. Something could have been dropped that was harder to detect except in narrow runtime circumstances.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

jedcunningham · 2024-12-14T00:00:04Z

That import was removed in #44839, but #44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

o-nikolas · 2024-12-16T17:56:57Z

Thanks @jedcunningham
I'm not sure how that happened or wasn't detected. Seems like a test should have failed but didn't? Are we missing coverage?

jedcunningham · 2024-12-16T19:41:22Z

No worries, this is just a standard "branch wasn't up to date with main, and tests run from the branch" scenario. Nothing missing really, just every so often we will have to fix these types of things.

potiuk · 2024-12-17T13:18:29Z

Cross-merging PRs. yeah.

And actually yes we could do something about it - that could go away if we had the possibility of using "Merge Queue" functionality: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue

The merge queue functionality works in the way, that instead of merging, committer adds PR to merge queue, and they are run in sequence - automatically rebasing them after the previously merged PR is merged - and only actually merging the PR when that "queued" PR had succeded again after rebase.

So far it was blocked as INFRA tooling is unable to retrieve "merger identity" for necessary ICLA "audit logs" when merge queue is used. But @davidarthur from Kafka team run a sandbox environment https://issues.apache.org/jira/browse/INFRA-25932 where he had shown how it is possible - so now it hangs a bit on INFRA making a decision whether to invest in it, implement missing piece and enable it.

If people here would like to comment on ths INFRA ticket and encourage it, having the possibility of using Merge Queues could help us to avoid this kind of issues, without impacting the velocity of merges.

o-nikolas · 2024-12-17T17:49:24Z

That sounds like an awesome mechanism @potiuk I added a +1 to the ticket (I didn't want to pollute the comment stream where details are being hashed out).

ashb · 2024-12-17T17:53:37Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

potiuk · 2024-12-17T19:41:53Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

Not when we go back and have ARC enabled, our tests elapsed time will go back to 20 minutes, and we already have done pretty damn good job (and continue doing so) on battling flaky tests. With our team effort we already have sometimes days without flaky tests, and yes while I would not want to use merge queues two months ago, I think we are pretty much ready to get it and get it much more stable (especially after we bring back 16 processor machines in our S3 to complete our tests 4 and sometimes even 8 times faster than the current builds. CC: @hussein-awala :D

potiuk · 2024-12-17T19:43:27Z

PLus merge queue will STILL use the selective checks optimizations - that is not going to change - so most of the PRS will be done in 10 minutes even without ARC (and maybe less than 4-5 minutes with ARC).

o-nikolas · 2024-12-17T20:55:05Z

I also think consistency is very important and worth paying a time cost for. Something could have been dropped that was harder to detect except in narrow runtime circumstances.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

jedcunningham · 2024-12-14T00:00:04Z

That import was removed in #44839, but #44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

o-nikolas · 2024-12-16T17:56:57Z

Thanks @jedcunningham
I'm not sure how that happened or wasn't detected. Seems like a test should have failed but didn't? Are we missing coverage?

jedcunningham · 2024-12-16T19:41:22Z

No worries, this is just a standard "branch wasn't up to date with main, and tests run from the branch" scenario. Nothing missing really, just every so often we will have to fix these types of things.

potiuk · 2024-12-17T13:18:29Z

Cross-merging PRs. yeah.

And actually yes we could do something about it - that could go away if we had the possibility of using "Merge Queue" functionality: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue

The merge queue functionality works in the way, that instead of merging, committer adds PR to merge queue, and they are run in sequence - automatically rebasing them after the previously merged PR is merged - and only actually merging the PR when that "queued" PR had succeded again after rebase.

So far it was blocked as INFRA tooling is unable to retrieve "merger identity" for necessary ICLA "audit logs" when merge queue is used. But @davidarthur from Kafka team run a sandbox environment https://issues.apache.org/jira/browse/INFRA-25932 where he had shown how it is possible - so now it hangs a bit on INFRA making a decision whether to invest in it, implement missing piece and enable it.

If people here would like to comment on ths INFRA ticket and encourage it, having the possibility of using Merge Queues could help us to avoid this kind of issues, without impacting the velocity of merges.

o-nikolas · 2024-12-17T17:49:24Z

That sounds like an awesome mechanism @potiuk I added a +1 to the ticket (I didn't want to pollute the comment stream where details are being hashed out).

ashb · 2024-12-17T17:53:37Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

potiuk · 2024-12-17T19:41:53Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

Not when we go back and have ARC enabled, our tests elapsed time will go back to 20 minutes, and we already have done pretty damn good job (and continue doing so) on battling flaky tests. With our team effort we already have sometimes days without flaky tests, and yes while I would not want to use merge queues two months ago, I think we are pretty much ready to get it and get it much more stable (especially after we bring back 16 processor machines in our S3 to complete our tests 4 and sometimes even 8 times faster than the current builds. CC: @hussein-awala :D

potiuk · 2024-12-17T19:43:27Z

PLus merge queue will STILL use the selective checks optimizations - that is not going to change - so most of the PRS will be done in 10 minutes even without ARC (and maybe less than 4-5 minutes with ARC).

o-nikolas · 2024-12-17T20:55:05Z

I also think consistency is very important and worth paying a time cost for. Something could have been dropped that was harder to detect except in narrow runtime circumstances.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

jedcunningham · 2024-12-14T00:00:04Z

That import was removed in #44839, but #44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

o-nikolas · 2024-12-16T17:56:57Z

Thanks @jedcunningham
I'm not sure how that happened or wasn't detected. Seems like a test should have failed but didn't? Are we missing coverage?

jedcunningham · 2024-12-16T19:41:22Z

No worries, this is just a standard "branch wasn't up to date with main, and tests run from the branch" scenario. Nothing missing really, just every so often we will have to fix these types of things.

potiuk · 2024-12-17T13:18:29Z

Cross-merging PRs. yeah.

And actually yes we could do something about it - that could go away if we had the possibility of using "Merge Queue" functionality: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue

The merge queue functionality works in the way, that instead of merging, committer adds PR to merge queue, and they are run in sequence - automatically rebasing them after the previously merged PR is merged - and only actually merging the PR when that "queued" PR had succeded again after rebase.

So far it was blocked as INFRA tooling is unable to retrieve "merger identity" for necessary ICLA "audit logs" when merge queue is used. But @davidarthur from Kafka team run a sandbox environment https://issues.apache.org/jira/browse/INFRA-25932 where he had shown how it is possible - so now it hangs a bit on INFRA making a decision whether to invest in it, implement missing piece and enable it.

If people here would like to comment on ths INFRA ticket and encourage it, having the possibility of using Merge Queues could help us to avoid this kind of issues, without impacting the velocity of merges.

o-nikolas · 2024-12-17T17:49:24Z

That sounds like an awesome mechanism @potiuk I added a +1 to the ticket (I didn't want to pollute the comment stream where details are being hashed out).

ashb · 2024-12-17T17:53:37Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

potiuk · 2024-12-17T19:41:53Z

Merge queue are a double edge sword, especially with our full test matrix taking ~1 hour and our sometimes flakey tests and it would greatly limit the number of PRs we could merge.

Not when we go back and have ARC enabled, our tests elapsed time will go back to 20 minutes, and we already have done pretty damn good job (and continue doing so) on battling flaky tests. With our team effort we already have sometimes days without flaky tests, and yes while I would not want to use merge queues two months ago, I think we are pretty much ready to get it and get it much more stable (especially after we bring back 16 processor machines in our S3 to complete our tests 4 and sometimes even 8 times faster than the current builds. CC: @hussein-awala :D

potiuk · 2024-12-17T19:43:27Z

PLus merge queue will STILL use the selective checks optimizations - that is not going to change - so most of the PRS will be done in 10 minutes even without ARC (and maybe less than 4-5 minutes with ARC).

o-nikolas · 2024-12-17T20:55:05Z

I also think consistency is very important and worth paying a time cost for. Something could have been dropped that was harder to detect except in narrow runtime circumstances.

That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

Import os in executor_loader
9222941
That import was removed in apache#44839, but apache#44710 wasn't up-to-date with main so static checks there didn't fail. This simply adds it back.

jedcunningham requested review from XD-DENG, ashb, hussein-awala, o-nikolas and pierrejeambrun as code owners December 14, 2024 00:00

boring-cyborgBot added the area:Executors-core LocalExecutor & SequentialExecutor label Dec 14, 2024

hussein-awala approved these changes Dec 14, 2024
View reviewed changes

jedcunningham merged commit 23f59fe into apache:mainDec 14, 2024

jedcunningham deleted the fix_ruff branch December 14, 2024 00:28