Open Stack

On 07/24/2017 08:05 AM, Michael Bayer wrote:
> On Sun, Jul 23, 2017 at 6:10 PM, Jay Pipes <jaypipes at 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-request at 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-request at 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-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>