Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd support for optimistic concurrency testing #102
Comments
This comment has been minimized.
This comment has been minimized.
Macarse
commented
Apr 6, 2017
|
@Eh2406 any update? |
This comment has been minimized.
This comment has been minimized.
|
I have not worked on it. Hopefully I will have time to get back to it. @edunham, how go the refactorings? |
This comment has been minimized.
This comment has been minimized.
|
@Eh2406 I've got it passing pep8 which was the first hurdle. The next set of changes should not affect any test results, and new tests would be absolutely wonderful for verifying the changes. Please add tests for anything and everything that you can think of! |
This comment has been minimized.
This comment has been minimized.
|
So this is a form of auto rollup, not tests of homu. Adding test of homu would be a laudable goal. Sorry for the confusing title. I am reading up on bors-ng and zuul. Trying to determine which approach requires less intrusive changes to implement and which lead to the biggest difference in understanding for our users. bors-ng does a optimistic rollup of all approved prs, if pass then merge else try a rollup of half the prs. This is a simple single threaded strategy. On the other hand the meaning of the queue will be different. zuul starts a separate branch for each pr in the queue this branch has the state the Next to look at the code to determine how hard this will be to implement. |
This comment has been minimized.
This comment has been minimized.
|
Ah, sorry for the confusion and thanks for clarifying. The most important thing to keep in mind when looking at how to stick others' optimistic testing schemes into Homu is that Servo's homu must never land a lower-priority PR when there is a max-priority PR in the queue. This wasn't previously a big deal but the feature is more essential now that it's on the way to becoming part of the Firefox sheriffs' workflow (see #111 for background). |
This comment has been minimized.
This comment has been minimized.
|
Yes I have been keeping that in mind, thanks for clarifying its importance. For zuul's approach there are no changes required. It still tests all changes separately and in order, just concurrently. For bors-ng 's approach I have been imagining a to clever by half solution. In which we split on the Do you have a link I could read on Gerrit's "fancier rollup logic"? It sounds relevant to this topic. |
This comment has been minimized.
This comment has been minimized.
|
For posterity, OpenStack has a project similar to bors/homu called Zuul which has a feature called 'gating' that seems relevant: |
This comment has been minimized.
This comment has been minimized.
|
Fundamentally Zuul is running all the same test as the current strategy, just running them in parallel. Unfortunately, rust lang "Right now we unfortunately just don't have capacity". So I have been focusing on adding procedurally generated rollups. After thinking through several complicated bisecting strategies I am leaning toward something small and conservative. There is also a good discussion of whether we even want procedurally generated rollups going on at internals. Perhaps we need some way to tell homu what should be considered for a rollup. |
This comment has been minimized.
This comment has been minimized.
notriddle
commented
Jun 1, 2017
|
I don't see how your approach is any better than just cutting in half, especially if the bad PR winds up in back. As for the priorities, it doesn't have to be complicated. You need to:
You might also want a max-batch-size option, but that's really orthogonal to the priority. |
This comment has been minimized.
This comment has been minimized.
|
Indeed it is strictly speaking worse than bisecting when measured by
Making sure all pr's in a rollup have the same priority, sounds like a conservative place to start. Thanks for that suggestion. The other two should be handled by homu's not having batches, just pr's and a queue. If a user wants to preempt the current rollup they can use a |
Eh2406 commentedMar 14, 2017
Recent discussion brought up suggestions for optimistic parallel testing.
We should look into allowing these features. I was going to start experimenting tonight, but @edunham mentioned incoming refactorings. So I thout I'd just leave an issue to remind me to come back to it.