Build properties #1380
Build properties #1380
Conversation
|
If it's big, where are the release notes? :) |
| from twisted.internet import reactor | ||
|
|
||
|
|
||
| class BProps(dict): |
There was a problem hiding this comment.
What's the purpose of this class?
There was a problem hiding this comment.
explicit casting. The concept is stolen from other properties table.
There was a problem hiding this comment.
not sure about it. dict(properties) is clearer to me.
There was a problem hiding this comment.
Agreed, less lines, less bugs, more coverage ;)
|
Re release-notes: It's not a change as we aim at providing the same functionality as for eight ... Not sure where to document that. |
|
But now the properties are stored in the database, right? So it's worth mentioning. |
|
New round ... |
|
👍 |
| @@ -403,9 +403,22 @@ def startNextStep(self): | |||
| self.executedSteps.append(s) | |||
| self.currentStep = s | |||
| d = defer.maybeDeferred(s.startStep, self.conn) | |||
| d.addCallback(self._flushProperties) | |||
There was a problem hiding this comment.
Anyway we could switch this method to an inlineCallback construction, and paralellize both _flushProperties and _stepDone ? Hint: the Errback makes it less trivial.
There was a problem hiding this comment.
You should flushProperties in case of error too. So this would be addBoth.
I dont think there is a need for unnecessary optimization.
|
Travis happy, let's wait for more pair of eyes, shall we ? |
|
Yes, I think it's a good idea. |
|
What a surprise ! The |
| @@ -116,6 +147,44 @@ def thd(conn): | |||
| results=results) | |||
| return self.db.pool.do(thd) | |||
|
|
|||
| @base.cached("BuildProperties") | |||
There was a problem hiding this comment.
Should we use a cache at all there ? Are we at risk of caching temporary values for a property, and returning the wrong (temporary) value to a subsequent call ?
There was a problem hiding this comment.
I really do not know. Maybe @tardyp could say something about this?
There was a problem hiding this comment.
yes, we should not cache anything unless the build is finished.
There was a problem hiding this comment.
Should I introduce a base.cacheIf/Unless ?
There was a problem hiding this comment.
For now, I would expect we use cache only for immutable. We will see with experience how we can enhance the caching behavior
| return BProps(l) | ||
| return self.db.pool.do(thd) | ||
|
|
||
| def setBuildProperty(self, bid, name, value, source): |
There was a problem hiding this comment.
Shall we have a id key which is hash(buildid, name) ? so that we can update the property directly?
http://dev.mysql.com/doc/refman/5.6/en/insert-on-duplicate.html
|
I think we should first merge the db part of this (model, migration, dbapi), which is most of the PR, and mostly okay |
|
I could do that, that would make the full story easier indeed.
|
|
See #1384 for the DB part of this PR. Hopefully it gets easier to review that way ... |
benallard
force-pushed
the
build_properties
branch
from
341b8f5
to
934c8b6
Compare
|
Just pushed the last part of this sequence here. |
|
I still have concern in the fact that this solution is unefficient, but it works, so lets merge it, and optimize it if the benchmarks shows it is needed. |
Here it is !
The underlying wiring is done: process -> data -> db.
I don't have data endpoint yet, it should make the subject of another PR I believe, That one is already big enough ...