On 07/24/2017 08:05 AM, Michael Bayer wrote:
On Sun, Jul 23, 2017 at 6:10 PM, Jay Pipes <jaypi...@gmail.com> wrote:
Glad you brought this up, Mike. I was going to start a thread about this.
Comments inline.
On 07/23/2017 05:02 PM, Michael Bayer wrote:
Well, besides that point (which I agree with), that is attempting to change
an existing database schema migration, which is a no-no in my book ;)
OK this point has come up before and I always think there's a bit of
an over-broad kind of purism being applied (kind of like when someone
says, "never use global variables ever!" and I say, "OK so sys.modules
is out right ?" :) ).
I agree with "never change a migration" to the extent that you should
never change the *SQL steps emitted for a database migration*. That
is, if your database migration emitted "ALTER TABLE foobar foobar
foobar" on a certain target databse, that should never change. No
problem there.
However, what we're doing here is adding new datatype logic for the
NDB backend which are necessarily different; not to mention that NDB
requires more manipulation of constraints to make certain changes
happen. To make all that work, the *Python code that emits the SQL
for the migration* needs to have changes made, mostly (I would say
completely) in the form of new conditionals for NDB-specific concepts.
In the case of the datatypes, the migrations will need to refer to
a SQLAlchemy type object that's been injected with the ndb-specific
logic when the NDB backend is present; I've made sure that when the
NDB backend is *not* present, the datatypes behave exactly the same as
before.
So basically, *SQL steps do not change*, but *Python code that emits
the SQL steps* necessarily has to change to accomodate for when the
"ndb" flag is present - this because these migrations have to run on
brand new ndb installations in order to create the database. If Nova
and others did the initial "create database" without using the
migration files and instead used a create_all(), things would be
different, but that's not how things work (and also it is fine that
the migrations are used to build up the DB).
There is also the option to override the compilation for the base
SQLAlchemy String type does so that no change at all would be needed
to consuming projects in this area, but it seems like there is a need
to specify ndb-specific length arguments in some cases so keeping the
oslo_db-level API seems like it would be best. (Note that the ndb
module in oslo_db *does* instrument the CreateTable construct globally
however, though it is very careful not to be involved unless the ndb
flag is present).
I guess the sitution is that if one is not using the ndb flag, the
python logic results in no SQL differences. And before these changes are
made it's not possible to run with the ndb flag - so there should be no
people for whom this is behavioral difference, right? (like, it's not
like we're going to have a person using the ndb flag missing an ndb
specific length somewhere because they ran the migrations before the
python logic was fixed, right?)
I can add these names up to oslo.db and then we would just need to
spread these out through all the open ndb reviews and then also patch
up Cinder which seems to be the only ndb implementation that's been
merged so far.
+1
Keep in mind this is really me trying to correct my own mistake, as I
helped design and approved of the original approach here where
projects would be consuming against the "ndb." namespace. However,
after seeing it in reviews how prevalent the use of this extremely
backend-specific name is, I think the use of the name should be much
less frequent throughout projects and only surrounding logic that is
purely to do with the ndb backend and no others. At the datatype
level, the chance of future naming conflicts is very high and we
should fix this mistake (my mistake) before it gets committed
throughout many downstream projects.
I had a private conversation with Octave on Friday. I had mentioned that I
was upset I didn't know about the series of patches to oslo.db that added
that module. I would certainly have argued against that approach. Please
consider hitting me with a cluestick next time something of this nature pops
up. :)
Also, as I told Octave, I have no problem whatsoever with NDB Cluster. I
actually think it's a pretty brilliant piece of engineering -- and have for
over a decade since I worked at MySQL.
My complaint regarding the code patch proposed to Nova was around the
hard-coding of the ndb namespace into the model definitions.
Best,
-jay
[1] https://review.openstack.org/#/c/427970/
[2] https://review.openstack.org/#/c/446643/
[3] https://review.openstack.org/#/c/446136/
__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
