aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Santhosh Kumar Shanmugham <santhoshkuma...@gmail.com>
Subject Re: Review Request 51384: Introduce UpdateMetadata fields in JobUpdateRequest.
Date Wed, 07 Sep 2016 17:26:50 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message