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.

Sign up for GitHub

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

Jump to bottom

Retrieve changes by branch #7090

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Retrieve changes by branch #7090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 11 additions & 2 deletions master/buildbot/data/buildsets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,17 @@ class BuildsetsEndpoint(Db2DataMixin, base.Endpoint):
def get(self, resultSpec, kwargs):
complete = resultSpec.popBooleanFilter('complete')
resultSpec.fieldMapping = self.fieldMapping
d = self.master.db.buildsets.getBuildsets(
complete=complete, resultSpec=resultSpec)

branch = resultSpec.popStringFilter('branch')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I am not sure if branch can be a specified field in the resultSpec. Since Buildset doesn't have a branch field, I think this might be an issue. In my custom run, I have also added a new entry in the fieldMapping but that translates into failing tests. So, can you please confirm if a resultSpec is limited to having exactly the same fields as the Buildset or not?

if branch is not None:
count = None
if resultSpec.limit:
count = resultSpec.limit
d = self.master.db.buildsets.getRecentBuildsets(complete=complete,
branch=branch, count=count)
else:
d = self.master.db.buildsets.getBuildsets(
complete=complete, resultSpec=resultSpec)

@d.addCallback
def db2data(buildsets):
Expand Down
13 changes: 11 additions & 2 deletions master/buildbot/data/changes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ def _fixChange(self, change, is_graphql):
{'name': k, 'source': v[1], 'value': json.dumps(v[0])}
for k, v in props.items()
]
change['builds'] = yield self.master.db.builds.getBuildsForChange(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be slow when retrieving many changes. Let's skip it for now, especially if you aren't using graphql.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see. Probably it does't make sense to always fetch them. But can it be at least added when branch is specified? The reason the builds are needed is to directly show them in the grid view without any additional calls. Maybe they can be merged in a single query. What do you think would be the best approach? Also, if there are two many changes, we can leave it out for now and I'll work on a new PR to add this.

change['changeid']
)
else:
sskey = ('sourcestamps', str(change['sourcestampid']))
change['sourcestamp'] = yield self.master.data.get(sskey)
Expand Down Expand Up @@ -104,8 +107,14 @@ def get(self, resultSpec, kwargs):
changes = []
else:
if resultSpec is not None:
resultSpec.fieldMapping = self.fieldMapping
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be missing something, but why can't we use fieldMapping for branch is not None case? Other potential filters may still be relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think other fields may be relevant. However, based on the table joins, some of them might have a different result which might be confusing since it will filter all the entries for a particular branch. Also, some of them might be trickier to integrate. I'll look into this in more detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on further looking, I think it should be possible to use the resultSpec, but that would be on top of the join between the changes and buildset tables, so some filters might have a different meaning between the case where 'branch' is specified or not. Is this ok?

changes = yield self.master.db.changes.getChanges(resultSpec=resultSpec)
branch = resultSpec.popStringFilter('branch')
if branch is not None:
changes = yield self.master.db.changes.getChangesForBranch(
branch, resultSpec.order, resultSpec.limit
)
else:
resultSpec.fieldMapping = self.fieldMapping
changes = yield self.master.db.changes.getChanges(resultSpec=resultSpec)
results = []
for ch in changes:
results.append((yield self._fixChange(ch, is_graphql='graphql' in kwargs)))
Expand Down
22 changes: 22 additions & 0 deletions master/buildbot/db/changes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,28 @@ def thd(conn):
def _getDataFromRow(self, row):
return row.changeid

@defer.inlineCallbacks
def getChangesForBranch(self, branch, order, count):
def thd(conn):
changes_tbl = self.db.model.changes
bsss_tbl = self.db.model.buildset_sourcestamps

from_clause = changes_tbl.join(bsss_tbl,
changes_tbl.c.sourcestampid == bsss_tbl.c.sourcestampid)
if branch == 'all':
q = sa.select([changes_tbl.c.changeid.distinct()]).select_from(
from_clause)
else:
q = sa.select([changes_tbl.c.changeid.distinct()]).select_from(
from_clause).where(changes_tbl.c.branch == branch)
q = q.limit(count).order_by(changes_tbl.c.changeid.desc())
rp = conn.execute(q)
changeids = [self._getDataFromRow(row) for row in rp]
rp.close()
return changeids
res = yield self.db.pool.do(thd)
return res

def getChanges(self, resultSpec=None):
def thd(conn):
# get the changeids from the 'changes' table
Expand Down