Some useful notes on how to use git review commands and options:
https://github.com/openstack-infra/git-review/blob/master/README.rst
- group by blueprint on Launchpad: https://blueprints.launchpad.net/ceilometer/+spec/api-group-by
- group by blueprint discussion on the OpenStack wiki: https://wiki.openstack.org/wiki/Ceilometer/blueprints/api-group-by
I noticed in the new storage tests I wrote for the class StatisticsGroupByTest
located in tests/storage/base.py,
-
For tests on group by and no period grouping, I checked the
groupbyattribute -
For tests on group by with period, I checked both the
groupbyattribute and theperiod_startattribute
This means that for queries with group by and no period grouping, the groupby
attribute should be a sufficient key, whereas for queries with both group by and
period, the combination of the groupby and period_start attributes should be
a sufficient key. The groupby and period_start attributes could be combined
into a tuple, and the tuple would be the key.
I mentioned to jd that I was worried about duplicate code in the map functions for the MongoDB connection.
Right now, in the MongoDB connection, two map functions are used to calculate
the meter statistics -- MAP_STATS and MAP_STATS_PERIOD
With the addition of group by functionality, you need two additional map
functions for doing group by without and with period: MAP_STATS_GROUPBY and MAP_STATS_PERIOD_GROUPBY
This is a total of 4 cases (map functions). I wonder if there is a way to
reduce the number of cases or at least reduce the amount of duplicated code. I
already see that there is quite a lot of duplicated code common to MAP_STATS
and MAP_STATS_PERIOD
jd says:
merging MAP_STATS and MAP_STATS_PERIOD could be a first patch
that'll help you for the future and will get you started
jd and I agreed that I could refactor the code to eliminate the duplicate code
that is common to both MAP_STATS and MAP_STATS_PERIOD.
The way the map function works in MongoDB/PyMongo is that the map function is
passed on bson.code.Code() as a string. So you can have a string that
contains the common code and use string replacement operations to account for
differences.
I refactored the map functions MAP_STATS and MAP_STATS_PERIOD in the
MongoDB driver and submitted this as a patch:
https://review.openstack.org/#/c/43043/
I was careful to use the appropriate Gerrit commands to submit this patch with the SQL Alchemy group by patch https://review.openstack.org/#/c/41597/10 as a dependency.
BSON.code.code() reference:
http://api.mongodb.org/python/current/api/bson/code.html
The following sequence of commands allowed me to submit my MongoDB map function refactoring patch with the SQL Alchemy group by patch as a dependency:
git fetch https://review.openstack.org/openstack/ceilometer refs/changes/97/41597/10 && git checkout FETCH_HEAD
git checkout -b bp/api-group-by
... make revisions ...
git add ceilometer/storage/impl_mongodb.py
git commit
... write commit message ...
git review -R
... this uploads the patch set to Gerrit ...
... make further revisions ...
git add <files>
git commit --amend
... update commit message ...
git review -R
... new patch set uploaded to Gerrit ...
Reference: https://wiki.openstack.org/wiki/Gerrit_Workflow#Add_dependency