Build properties db #1384
Build properties db #1384
Conversation
| for row in conn.execute(q): | ||
| try: | ||
| prop = (json.loads(row.value), row.source) | ||
| except ValueError: |
|
Re JSON: There has been a deprecation message for non-JSON-izable property values (for how long ?), so maybe we could just refuse them ... |
|
Once again: release notes. |
|
This PR is doing nothing, just adding (yet unused) methods and table ... I'd put a message in the release note in one of the next ones ... One that actually does something ... |
|
Where did you see this deprecation? (I can't find it easily.) |
|
Why one would do that? Why not start using this functionality right away? |
|
deprecation: buildbot/process/properties.py:142 |
|
Looks like Travis is happy ! yeah ! \o/ |
|
re deprecation. Thanks. I checked the revision when it was introduced, and it was post 0.8.8 (0.8.9). Since 0.9.0 is a major change, I think it's good time to become strict. @djmitche, @tardyp what are you thoughts? re unused code: I understand that it's only part of the whole change, but I feel uneasy about introducing only one part, which essentially does nothing. I'd leave others to comment on this. |
|
re: deprecation - nine is what that deprecation notice was looking forward to, so by all means disable it re: unused code - we've gotten ourselves in trouble before accepting part of an unfinished project. However, that was from less dedicated souls than Ben, so I'm inclined to allow it here. We just need to remember to back it out if for some reason the project doesn't get finished. |
|
Thanks Dustin ! And just remember: if you want the full thing, just merge #1380. That one's working. I'm all for easier review, hence the split. I just hope this will help us get that one in a bit quicker.
|
|
GH-1380 can't be merged at the moment. Probably conflicts. |
|
@sa2ajj re dead code: I am the one that suggested ben to split is work, so that it is manageable. First PR: DB We have been doing that already for the rest of the data model. GH-1380 is more a proof of concept, and we'll try to make it perfect over the next weeks |
|
Ok. |
| build_properties = sa.Table('build_properties', metadata, | ||
| sa.Column('buildid', sa.Integer, sa.ForeignKey('builds.id'), | ||
| nullable=False), | ||
| sa.Column('name', sa.String(256), nullable=False), |
There was a problem hiding this comment.
I would like that we try to create a unique key on that table.
The unique key would be something like int(sha1(buildid + name))
There was a problem hiding this comment.
I'm still unsure what this would be good for ? indexing ? performance ?
There was a problem hiding this comment.
Write performance.
With this, you can do all the inserts in one query instead of 2*n queries.
Even if this is not used in the first impl, this would allow the optimization without yet another migration script.
There was a problem hiding this comment.
Well, the 'yet another migration script' is a bit burned now ...
|
Oops :((( I already merged it for some unobvious reason. |
| call """ | ||
| def thd(conn): | ||
| bp_tbl = self.db.model.build_properties | ||
| self.checkLength(bp_tbl.c.name, name) |
There was a problem hiding this comment.
I tried adding tests for those checkLength, turned out they look like noop to me. Any hints there ?
There was a problem hiding this comment.
@djmitche ?
Maybe in unit tests checkLength is no-op for fakedb?
There was a problem hiding this comment.
Even for the RealDB, I had the same results ... It's probably only implemented for mysql, and the base class has tests for them.
There was a problem hiding this comment.
Yes, checkLength exists just to avoid MySQL's ever-so-helpful habit of silently truncating inputs. Other DB engines will either store the entire input, or give an error.
|
So what's happening here ? Are we still gathering remarks there ? or should someone revert it. :/!: There is a migrator included :/!: |
|
Re: JSON - See #1385. |
This is a full rewrite of #1380 (only the db part), as my git-foo is not good enough to let me do that kind of magic. So please review it carefully.
All data and process stuff has been left out.
I'm keeping #1380 around as reference.