You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/07/01 18:55:36 UTC
Re: Review Request 23105: Database-backed implementation of AttributeStore.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23105/#review47077
-----------------------------------------------------------
src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml
<https://reviews.apache.org/r/23105/#comment82688>
Do you really need this block? I thought EnumValueMapper takes care of this already.
src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml
<https://reviews.apache.org/r/23105/#comment82692>
Pretty sure you can omit id column here.
src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml
<https://reviews.apache.org/r/23105/#comment82684>
Should this be a MERGE instead to simplify re-population case? Otherwise, I am not sure how a re-run of the population loop in DBStorage is going to be handled here.
src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql
<https://reviews.apache.org/r/23105/#comment82680>
You probably want UNIQUE(name) here as well.
src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql
<https://reviews.apache.org/r/23105/#comment82678>
Is there a legitimate case when UNIQUE(host, slave_id) would not suffice here?
- Maxim Khutornenko
On June 27, 2014, 3:39 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23105/
> -----------------------------------------------------------
>
> (Updated June 27, 2014, 3:39 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-557
> https://issues.apache.org/jira/browse/AURORA-557
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There are a few different characteristics of this mapper compared to others so far:
> - custom type handler (see AbstractTEnumTypeHandler and MaintenanceModeTypeHandler)
> - outer join (to allow a HostAttributes with an empty Attributes set)
> - batch insert via foreach
> - collection, and nested collection result mapping
>
> You may find this page helpful to explain the features used: http://mybatis.github.io/mybatis-3/sqlmap-xml.html#Result_Maps
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/AttributeMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/DbAttributeStore.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 1738b95cd67cf990bd8aad8c744a1febe2d87f15
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java c683e398640c7ebf2047ef308a701cb4897c58dc
> src/main/java/org/apache/aurora/scheduler/storage/db/EnumValueMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 505c94d6800c1453b1b1f696ef774f5943973f19
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTEnumTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/MaintenanceModeTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStore.java 4bb807c04e47a091c83a575850ebfc3b244bfa73
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 65750b61b864f0e830513039a7c9d727ac9d493d
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 23555c2483d7fe716243847f8478898e98fb5ac4
> src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 31b98cb3107a88756694922de01fa0ba267f3e9d
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 3298eb38644b6fa7096801a69f8b88d0331ce4a7
> src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23105/diff/
>
>
> Testing
> -------
>
> ./gradlew run -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 23105: Database-backed implementation of AttributeStore.
Posted by Kevin Sweeney <ke...@apache.org>.
> On July 1, 2014, 9:55 a.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql, lines 58-59
> > <https://reviews.apache.org/r/23105/diff/1/?file=619262#file619262line58>
> >
> > Is there a legitimate case when UNIQUE(host, slave_id) would not suffice here?
I think you need to drop the UNIQUE(host) constraint - only slave_id is necessarily unique. multiple mesos slaves could have the same hostname, and slave_id is persistent across slave restarts that don't result in reboots / recovery info corruption (at which point you want to invalidate attribute storage anyway since they'll potentially have changed)
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23105/#review47077
-----------------------------------------------------------
On June 27, 2014, 8:39 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23105/
> -----------------------------------------------------------
>
> (Updated June 27, 2014, 8:39 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-557
> https://issues.apache.org/jira/browse/AURORA-557
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There are a few different characteristics of this mapper compared to others so far:
> - custom type handler (see AbstractTEnumTypeHandler and MaintenanceModeTypeHandler)
> - outer join (to allow a HostAttributes with an empty Attributes set)
> - batch insert via foreach
> - collection, and nested collection result mapping
>
> You may find this page helpful to explain the features used: http://mybatis.github.io/mybatis-3/sqlmap-xml.html#Result_Maps
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/AttributeMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/DbAttributeStore.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 1738b95cd67cf990bd8aad8c744a1febe2d87f15
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java c683e398640c7ebf2047ef308a701cb4897c58dc
> src/main/java/org/apache/aurora/scheduler/storage/db/EnumValueMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 505c94d6800c1453b1b1f696ef774f5943973f19
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTEnumTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/MaintenanceModeTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStore.java 4bb807c04e47a091c83a575850ebfc3b244bfa73
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 65750b61b864f0e830513039a7c9d727ac9d493d
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 23555c2483d7fe716243847f8478898e98fb5ac4
> src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 31b98cb3107a88756694922de01fa0ba267f3e9d
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 3298eb38644b6fa7096801a69f8b88d0331ce4a7
> src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23105/diff/
>
>
> Testing
> -------
>
> ./gradlew run -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 23105: Database-backed implementation of AttributeStore.
Posted by Bill Farner <wf...@apache.org>.
> On July 1, 2014, 4:55 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml, lines 6-14
> > <https://reviews.apache.org/r/23105/diff/1/?file=619259#file619259line6>
> >
> > Do you really need this block? I thought EnumValueMapper takes care of this already.
Thank you. This is pre-refactor cruft. Removed.
> On July 1, 2014, 4:55 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml, line 18
> > <https://reviews.apache.org/r/23105/diff/1/?file=619259#file619259line18>
> >
> > Pretty sure you can omit id column here.
Right you are, removed.
> On July 1, 2014, 4:55 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml, line 7
> > <https://reviews.apache.org/r/23105/diff/1/?file=619260#file619260line7>
> >
> > Should this be a MERGE instead to simplify re-population case? Otherwise, I am not sure how a re-run of the population loop in DBStorage is going to be handled here.
The re-population case crossed my mind, but i figured it would better for it to fail early if that happens. Otherwise you run the risk of crufty deleted values in the database. In other words, if this code is used for a persistent database, i want it to break so that it must be re-thought.
> On July 1, 2014, 4:55 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql, lines 48-49
> > <https://reviews.apache.org/r/23105/diff/1/?file=619262#file619262line48>
> >
> > You probably want UNIQUE(name) here as well.
Good call, done.
> On July 1, 2014, 4:55 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql, lines 58-59
> > <https://reviews.apache.org/r/23105/diff/1/?file=619262#file619262line58>
> >
> > Is there a legitimate case when UNIQUE(host, slave_id) would not suffice here?
>
> Kevin Sweeney wrote:
> I think you need to drop the UNIQUE(host) constraint - only slave_id is necessarily unique. multiple mesos slaves could have the same hostname, and slave_id is persistent across slave restarts that don't result in reboots / recovery info corruption (at which point you want to invalidate attribute storage anyway since they'll potentially have changed)
I wanted to avoid changing existing API semantics here, but position internally for it. That's why i've made the foreign key use slave_id rather than host. However, i don't want to risk having a semantic change throw a wrench in this refactor. For that reason, i used UNIQUE(host) for matching behavior.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23105/#review47077
-----------------------------------------------------------
On June 27, 2014, 3:39 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23105/
> -----------------------------------------------------------
>
> (Updated June 27, 2014, 3:39 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-557
> https://issues.apache.org/jira/browse/AURORA-557
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There are a few different characteristics of this mapper compared to others so far:
> - custom type handler (see AbstractTEnumTypeHandler and MaintenanceModeTypeHandler)
> - outer join (to allow a HostAttributes with an empty Attributes set)
> - batch insert via foreach
> - collection, and nested collection result mapping
>
> You may find this page helpful to explain the features used: http://mybatis.github.io/mybatis-3/sqlmap-xml.html#Result_Maps
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/AttributeMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/DbAttributeStore.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 1738b95cd67cf990bd8aad8c744a1febe2d87f15
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java c683e398640c7ebf2047ef308a701cb4897c58dc
> src/main/java/org/apache/aurora/scheduler/storage/db/EnumValueMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 505c94d6800c1453b1b1f696ef774f5943973f19
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTEnumTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/MaintenanceModeTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStore.java 4bb807c04e47a091c83a575850ebfc3b244bfa73
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 65750b61b864f0e830513039a7c9d727ac9d493d
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 23555c2483d7fe716243847f8478898e98fb5ac4
> src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 31b98cb3107a88756694922de01fa0ba267f3e9d
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 3298eb38644b6fa7096801a69f8b88d0331ce4a7
> src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23105/diff/
>
>
> Testing
> -------
>
> ./gradlew run -Pq
>
>
> Thanks,
>
> Bill Farner
>
>