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 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
