On 02/02/2012 05:30 PM, Andy Doan wrote:
> Starting a new thread to deal with this project.
>
> We now have this project hosted at:
>
>
> <http://android.git.linaro.org/gitweb?p=lava-server/android_benchmark_views.…>
>
> I've made a number of improvements since this was initially sent out
> including:
Hi Andy.
I'm reading the code now. I have some comments. Essentially before this
lands you _have_ to add one change, then we can follow up with iterative
improvements.
You _have_ to apply south migrations to your application. Without that
we will not be able to make reasonable upgrades. Adding migrations later
is not an option as there is a bit that is very hard to automate.
Fortunately this is pretty easy:
1) Add a dependency on south (install_requires in setup.py)
2) Run lava-server manage ... android_benchmark_views_app
schemamigration --initial --auto
3) Inspect the code briefly and add it to version control
4) Push that up for review.
As for the rest:
*) helpers.py, don't use double leading underscore.
*) Try keeping the code that relates to your model in the model class,
also add a few one liners if you can to document each public method
*) extension.py, in def version(), pass the second argument to
versiontools.format_version() please pass your main package object
(android_benchmark_views_app). This will make it follow git version hashes.
*) models.py, BenchmarkRun, each ForeignKey should have a related_name.
This name is used when accessing the model field from the "remote" end,
then you can navigate back using this name. In general this is required
in some cases (when there are clashes, anyway this is beyond the scope
of this conversation)
*) models.py BenchmarkReport, don't use comments like that. Firs of all,
in django it's very very bad to use nullable text/char fields. Instead
always use null=False, blank=True - that makes writing code then saner.
As for comments. Instead of using a simple text field please consider
using django comment system. It _is_ slightly more complicated than
using a free-for-all text area (like on launchpad's whiteboard but at
the same time you get much more in return. You can read about this here:
https://docs.djangoproject.com/en/dev/ref/contrib/comments/
*) models.py both classes, add a get_absolute_url() method based on
django permalink system. It's trivial to use, see this document:
https://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.model…
*) You ship query.flot. While that's okay for now I'd rather see this
managed by a dedicated django app that just wraps this in one place. As
for now this can stay as-is.
*) Your templates extend dashboard templates. Please don't do that (as
those templates are not a stable/public interface). Instead extend
lava-server templates that work just as well but are stable.
That's a very quick review. I have one more comment.
EXCELLENT WORK :-)
Could you please join the validation team? :-)
Thanks
ZK
>
> * general improvements to the flot library for rendering things
> * zoom support for graphs
> * DB queries
>
> The DB queries is the main thing Zygmunt complained about from the
> initial code. I now hit the DB with one query to get all the
> measurements I need it and let it perform the StdDev/Avg[1]
> calculations. The performance (at least on my laptop) seems pretty good
> now. The DB query improvement also allowed me to get rid of some ugly code.
>
> I'm hoping going forward I can start doing submissions as code reviews
> in Gerrit and have someone on this team look at each change.
>
> Let me know what you think - or we can always sit down and chat about it
> directly next week.
>
> [1]: NOTE: StdDev/Avg functions aren't supported by Sqlite3, so this
> feature only works with postgres/mysql.
>
> -andy
--
Zygmunt Krynicki
Linaro Validation Team
Hi folks.
I've noticed bootstrap [1] and it seems quite interesting as a UI
toolkit. It seems more featureful and consistent than jQuery UI.
Perhaps this is something to look at when we need to build/rebuild UI
elements.
Best regards
ZK
[1]: http://twitter.github.com/bootstrap/
I had a request today to produce some information about how long each board
spends running jobs, idling, or offline as percentages for the past month,
quarter, etc. This will be even more useful as we start doing things like
offlining boards automatically when there is a problem.
https://blueprints.launchpad.net/lava-scheduler/+spec/lava-board-state-logg…
I know it won't be feasible to get historical data for offlinined boards,
but I'd like if we could make it possible from this point forward. So this
might be a good thing to try to get done during the hacking sessions this
week.
Thanks,
Paul Larson
---------- Forwarded message ----------
From: Andy Doan
Date: 2 February 2012 18:30
Subject: code review: lava android benchmark code
Starting a new thread to deal with this project.
We now have this project hosted at:
<http://android.git.linaro.org/gitweb?p=lava-server/android_benchmark_views.…>
I've made a number of improvements since this was initially sent out
including:
* general improvements to the flot library for rendering things
* zoom support for graphs
* DB queries
The DB queries is the main thing Zygmunt complained about from the
initial code. I now hit the DB with one query to get all the
measurements I need it and let it perform the StdDev/Avg[1]
calculations. The performance (at least on my laptop) seems pretty good
now. The DB query improvement also allowed me to get rid of some ugly code.
I'm hoping going forward I can start doing submissions as code reviews
in Gerrit and have someone on this team look at each change.
Let me know what you think - or we can always sit down and chat about it
directly next week.
[1]: NOTE: StdDev/Avg functions aren't supported by Sqlite3, so this
feature only works with postgres/mysql.
-andy
--
Fathi Boudra
Linaro Release Manager | Validation Project Manager
Linaro.org | Open source software for ARM SoCs
