Mailing-List: contact reviews-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============5626428059893551606=="
MIME-Version: 1.0
Subject: Re: Review Request 24521: Finalizing DB mapper implementation.
From: "Maxim Khutornenko" <maxim@apache.org>
To: "Bill Farner" <wfarner@apache.org>,
 "David McLaughlin" <david@dmclaughlin.com>
Cc: "Aurora" <reviews@aurora.incubator.apache.org>,
 "Maxim Khutornenko" <maxim@apache.org>
Date: Mon, 11 Aug 2014 18:04:41 -0000
Message-ID: <20140811180441.10843.94560@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Maxim Khutornenko" <noreply@reviews.apache.org>
References: <20140809004850.11985.88076@reviews.apache.org>
In-Reply-To: <20140809004850.11985.88076@reviews.apache.org>
Reply-To: "Maxim Khutornenko" <maxim@apache.org>

--===============5626428059893551606==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


> On Aug. 9, 2014, 12:48 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 40
> > <https://reviews.apache.org/r/24521/diff/2/?file=656929#file656929line40>
> >
> >     Drive-by partial review, but this seems like an API regression.  Do you expect duplicates in the result?
> 
> Maxim Khutornenko wrote:
>     This is the only way to preserve ordering in the result set. I am happy to revert it if we decide to sort on the client. It's a bit weird to support paging with partial results coming out of order though.
> 
> David McLaughlin wrote:
>     +1 for keeping it a list.
> 
> Bill Farner wrote:
>     We can guarantee order with a Set (ImmutableSet.copyOf does this).  I do think it's fair to force the client to sort if they want to guarantee sorted order though (especially since we don't accept a sort directive).

The order will not be guaranteed with python client when data gets through thrift serialization. We should either keep List and document the order by lastModifiedTS or revert to Set and drop any guarantees. I am fine either way. 


- Maxim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24521/#review50098
-----------------------------------------------------------


On Aug. 11, 2014, 5:45 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24521/
> -----------------------------------------------------------
> 
> (Updated Aug. 11, 2014, 5:45 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-612
>     https://issues.apache.org/jira/browse/AURORA-612
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Adding support for storing task configs and updateOnlyTheseInstances option.
> Implementing the rest of defined JobUpdateStore APIs.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java a9a325b877898be8a265c45112757deba0c3583f 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 808595817c31f3ef3515b623bbeb575bbf1f73fe 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87f428be892204b8149170d39d3ace2962abc4bd 
>   src/main/java/org/apache/aurora/scheduler/storage/db/InsertResult.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java 5b7a3df035e78832ba4e6b595202d06af5d664a5 
>   src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTBaseTypeHandler.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TaskConfigTypeHandler.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java c5468b16709e7bc4758a0597bbe14257b31686ab 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml ce4912d70fe75ea97f3d1bfb7fc4d5ec437fc83b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 3c69bda9fcb473205a3e308b15ad18cd1ae3cba7 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql f0c8336cde4d262bcaf99921c556875ba819b05c 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 4ea9ec2b96fc12429f33336b53e677e12662ec9a 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java c76ab5cbde907a352dce25da585951831733487d 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 548322be029c6663a75187d9f341e53c5b7ca416 
>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java cbb9c468bf64691e573762f3734528c7b2e16490 
> 
> Diff: https://reviews.apache.org/r/24521/diff/
> 
> 
> Testing
> -------
> 
> gradle -Pq build
> ./pants src/test/python/apache/aurora/client/api:scheduler_client
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>


--===============5626428059893551606==--