> On Sept. 2, 2016, 2:37 p.m., Maxim Khutornenko wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 774
> > <https://reviews.apache.org/r/51384/diff/7/?file=1489637#file1489637line774>
> >
> >     nit: all comments should be ended with '.'. Here and everywhere.

Fixed.


> On Sept. 2, 2016, 2:37 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java, 
> > line 102
> > <https://reviews.apache.org/r/51384/diff/7/?file=1489638#file1489638line102>
> >
> >     `checkNotBlank` is redundant here as you enter this block only if it's 
> > not empty.

Fixed.


> On Sept. 2, 2016, 2:37 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  lines 932-934
> > <https://reviews.apache.org/r/51384/diff/7/?file=1489641#file1489641line932>
> >
> >     tabbing is off here

Fixed.


> On Sept. 2, 2016, 2:37 p.m., Maxim Khutornenko wrote:
> > src/main/python/apache/aurora/client/cli/context.py, line 79
> > <https://reviews.apache.org/r/51384/diff/7/?file=1489645#file1489645line79>
> >
> >     This is only used in update.py, please move it there.

Fixed.


> On Sept. 2, 2016, 2:37 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 913
> > <https://reviews.apache.org/r/51384/diff/7/?file=1489641#file1489641line913>
> >
> >     This will be a null ref if scheduler is called from a v-1 client that 
> > does not know anything about update metadata. You need to do something like 
> > this:
> >     ```
> >     .setMetadata(request.isSetMetadata() 
> >         ? toBuildersSet(request.getMetadata()) 
> >         : ImmutableSet.of())
> >     ```

Thrift does not expose isSetMetadata() method for optional fileds of primitive 
type in the IJobUpdateRequest. It is available however in JobUpdateRequest.java 
and the earlier code that converts from JobUpdateRequest to IJobUpdateRequest 
does the check, via IJobUpdateRequest.build().

// SchedulerThrift.java
      request = IJobUpdateRequest.build(new 
JobUpdateRequest(mutableRequest).setTaskConfig(
          configurationManager.validateAndPopulate(
              ITaskConfig.build(mutableRequest.getTaskConfig())).newBuilder()));
              
// IJobUpdateRequest.java
  private IJobUpdateRequest(JobUpdateRequest wrapped) {
    ...
    this.metadata = wrapped.isSetMetadata()
        ? FluentIterable.from(wrapped.getMetadata())
              .transform(IMetadata::build)
              .toSet()
        : ImmutableSet.<IMetadata>of();
  }
  
  public static IJobUpdateRequest build(JobUpdateRequest wrapped) {
    return new IJobUpdateRequest(wrapped);
  }


> On Sept. 2, 2016, 2:37 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml,
> >  lines 357-358
> > <https://reviews.apache.org/r/51384/diff/7/?file=1489648#file1489648line357>
> >
> >     Instead of sub-selecting from a left join, have you considered a 
> > collection with a subselect instead? See `details.updateEvents` for 
> > example. 
> >     
> >     It should be more compact and most importantly, perform better on large 
> > metadata counts. You can validate the last assumption via `./gradlew jmh 
> > -Pbenchmarks='UpdateStoreBenchmarks.JobDetailsBenchmark'` or create a new 
> > benchmark to test fetching job summaries only.

Updated to use a sub-select instead of the join.

Added a benchmark test. Avoiding joins and using sub-selects appear to help out 
in the throughput. Beyond 5000 metadata pairs the throughput flattens out to 1 
op/sec.


- Santhosh Kumar


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


On Sept. 6, 2016, 8:29 p.m., Santhosh Kumar Shanmugham wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51384/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2016, 8:29 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Joshua Cohen, and Maxim 
> Khutornenko.
> 
> 
> Bugs: AURORA-1711
>     https://issues.apache.org/jira/browse/AURORA-1711
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Allow custom metadata to be stored with the job updates. This feature
> is to help case the reconciliation on job update request on the clients.
> 
> Today when a client's requests are proxied via a middle-man,
> if the middle-man times out before the scheduler responds with success,
> the client is left the only option of retry. In the case of updates,
> since multiple updates are not allowed in parallel, the retries fail,
> with INVALID_REQUEST. Although the original update is already accepted
> by the scheduler and is in-progress.
> 
> With this feature, clients can reconcile the job updates,
> - by marking updates them with a unique id in the metadata field
> - scheduler returns the in-progress job update in its response
> - client can match the client-generated ids to reconcile its state.
> 
> 
> Diffs
> -----
> 
>   RELEASE-NOTES.md d79aaadc197697d09a71c83494a01765d6a983d4 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   src/jmh/java/org/apache/aurora/benchmark/JobUpdates.java 
> f4f8d0037751c9c2096747264c19f6292461b308 
>   src/jmh/java/org/apache/aurora/benchmark/UpdateStoreBenchmarks.java 
> e5228ae9baadb8cad1e5ce143df09426d6107583 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> d2673e6b328cb1e249fbe91d18e0d9e935636eaa 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java
>  a3b04949f1726f110638e4f93ef45947cdb9e7f8 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/migration/V008_CreateUpdateMetadataTable.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  594bb6219294dcc77d48dcad14e2a6f9caa0c534 
>   
> src/main/java/org/apache/aurora/scheduler/updater/UpdateInProgressException.java
>  PRE-CREATION 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   src/main/python/apache/aurora/client/cli/update.py 
> d23243dcf93dd82f66b4c8cc4342bd2c8d2adc79 
>   src/main/python/apache/aurora/client/hooks/hooked_api.py 
> 542f165add0f1d01a782fe6d6089bff3e736fb82 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
>  8563630a179cb6d1e3b0e22c730ccbfe1c9291e2 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> a40830fd668aa650c22a48cbc757b45aef27e961 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  08530397ff75081bde6f07f9d53317b5486e0da4 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  a7d1f74acdfe5f58eb822a4d826e920cad53dced 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> 04551f17999d742c53dfb1b36286b119b448550e 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> 92fb039fb7d3e368d7c0dfa5ebdb465c7f112416 
>   src/test/python/apache/aurora/client/cli/util.py 
> aac9f9c802e4ad1f06cee8cf3f56749760b33af5 
> 
> Diff: https://reviews.apache.org/r/51384/diff/
> 
> 
> Testing
> -------
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ./pants test src/test/python/apache/aurora/client/cli:cli
> 
> 
> Thanks,
> 
> Santhosh Kumar Shanmugham
> 
>

Reply via email to