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

buildRequestCollapsing not working in 0.9.6 as expected #3151

Closed
sphet opened this Issue Apr 26, 2017 · 8 comments

Comments

Projects
None yet
5 participants
@sphet
Contributor

sphet commented Apr 26, 2017

I am trying to get build request merging to work in 0.9 - we are using it effectively in 0.8.6. The documentation states:

Sourcestamps are compatible if all of the below conditions are met:

Their codebase, branch, project, and repository attributes match exactly
Neither source stamp has a patch (e.g., from a try scheduler)
Either both source stamps are associated with changes, or neither are associated with changes but they have matching revisions.

In my case I have a long-running builder - it takes five minutes to complete - and I submit the following sequence of changes to Perforce:

edit & submit fileA.txt at 10:00AM ; CL461078 --> builder starts
edit & submit fileB.txt at 10:02AM ; CL461079 --> builder is busy, submission is pending
edit & submit fileC.txt at 10:03AM ; CL461080 --> builder is busy, submission is pending

Once CL001 is finished building, I would expect CL002 and CL003 to be collapsed into one build request. This is not happening. I have configued the builder with 'collapseRequests=True'. The debug output shown for the source stamps is:

[{"branch":null,"codebase":"","created_at":1493227979,"patch":null,"project":"","repository":"","revision":"461078","ssid":427}]
[{"branch":null,"codebase":"","created_at":1493228009,"patch":null,"project":"","repository":"","revision":"461079","ssid":428}]
[{"branch":null,"codebase":"","created_at":1493228039,"patch":null,"project":"","repository":"","revision":"461080","ssid":429}]

Looking at the source code I don't really see any code that would handle the 'both source stamps are associated with changes', as certainly in the canBeCollapsed function in buildRequest.pyL254 the collapse is failing because the revisions are different.

Is my reading of the documentation wrong? In 8.6 it seemed this worked - stacked up pending requests (regardless of revision number) would all be built once the builder is free, not atomically. How would we modify the code to handle this?

Reviewing the source code to 0.8.x I see that in sourcestamp.py the canBeMergedWith reads, in addition to other things:

    if self.changes and other.changes:
        return True
    elif self.changes and not other.changes:
        return False  # we're using changes, they aren't
    elif not self.changes and other.changes:
        return False  # they're using changes, we aren't

I don't see parallel code in the 0.9 branch.

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Apr 26, 2017

Member

Indeed, problem comes from this very old change from the begining of nine refactor:
a3fcc91#diff-e28bcb3c5230c2d392829647506762daL169

The new algorithm does not implement the condition documented: "Either both source stamps are associated with changes"

When looking at the coverage for this code, it looks like there is no unit test for it.

Member

tardyp commented Apr 26, 2017

Indeed, problem comes from this very old change from the begining of nine refactor:
a3fcc91#diff-e28bcb3c5230c2d392829647506762daL169

The new algorithm does not implement the condition documented: "Either both source stamps are associated with changes"

When looking at the coverage for this code, it looks like there is no unit test for it.

@tardyp tardyp added this to the 1.0.0 milestone Apr 26, 2017

@sphet

This comment has been minimized.

Show comment
Hide comment
@sphet

sphet Apr 26, 2017

Contributor

I am working through the code now to see if I sort it out, but it seems the changes are associated with a builder's source, and not sourcestamps. Given the two build requests passed to canBeCollapsed, I think I can query the sources and go from there. I'll give it a try.

Contributor

sphet commented Apr 26, 2017

I am working through the code now to see if I sort it out, but it seems the changes are associated with a builder's source, and not sourcestamps. Given the two build requests passed to canBeCollapsed, I think I can query the sources and go from there. I'll give it a try.

@noc0lour

This comment has been minimized.

Show comment
Hide comment
@noc0lour

noc0lour Apr 26, 2017

Contributor

For testing you could write a unit test and put prints into the source, so you don't have to run a test-master. That's how I test changes like this often ;)
Just install buildbot in a virtualenv with pip install -e master

Contributor

noc0lour commented Apr 26, 2017

For testing you could write a unit test and put prints into the source, so you don't have to run a test-master. That's how I test changes like this often ;)
Just install buildbot in a virtualenv with pip install -e master

@sphet

This comment has been minimized.

Show comment
Hide comment
@sphet

sphet Apr 26, 2017

Contributor

I am not sure if my patch is sufficiently safeguarded, but I ran it in a couple of test environments and it seems to work. I do not know the code base enough - perhaps a moderator can look it over..

Contributor

sphet commented Apr 26, 2017

I am not sure if my patch is sufficiently safeguarded, but I ran it in a couple of test environments and it seems to work. I do not know the code base enough - perhaps a moderator can look it over..

@vigsterkr

This comment has been minimized.

Show comment
Hide comment
@vigsterkr

vigsterkr Jun 27, 2017

any update on this one as i'm experiencing the same problem on 0.9.8

vigsterkr commented Jun 27, 2017

any update on this one as i'm experiencing the same problem on 0.9.8

@sphet

This comment has been minimized.

Show comment
Hide comment
@sphet

sphet Jun 27, 2017

Contributor
Contributor

sphet commented Jun 27, 2017

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Jun 28, 2017

Member

@sphet what do you mean by a good setup for writing the unit tests?

did you see my answer here?
#3152 (comment)

what could I have done better to help you?

Member

tardyp commented Jun 28, 2017

@sphet what do you mean by a good setup for writing the unit tests?

did you see my answer here?
#3152 (comment)

what could I have done better to help you?

@sphet

This comment has been minimized.

Show comment
Hide comment
@sphet

sphet Jun 28, 2017

Contributor
Contributor

sphet commented Jun 28, 2017

@tardyp tardyp closed this Nov 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment