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.git;a=summary
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.models... *) 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