aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stephan Erb <s...@apache.org>
Subject Re: Review Request 57061: Enable Mesos HTTP API.
Date Mon, 27 Feb 2017 23:22:07 GMT

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



In general this looks good to me. I am not an expert on the HTTP API though, so I cannot really
say if we are using it correctly in all corner cases. I trust your judgement here :-)

However, I have to admit I am not super thrilled about the merged SchedulerDriver. The new
HTTP API is the one to stay in the future, so by keeping it separate we would have (potentially)
achieved a cleaner code base and would have made it simpler to drop the old API in the future.
While I achknowledge that this approach has reduced duplication, I don't believe it outweights
the added complexity of the merged implementation.

I am happy to here what you think about this.


src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java (lines 97 - 98)
<https://reviews.apache.org/r/57061/#comment239051>

    Please already file a ticket and link it here, so that we can keep track of the (upcoming)
deprecation.



src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java (line 353)
<https://reviews.apache.org/r/57061/#comment239052>

    Would be great if we could log more info about the master we have connected to.



src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java (lines 443 - 444)
<https://reviews.apache.org/r/57061/#comment239053>

    Should we add some debug logging here? Or is this too frequent even for debug?



src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java (line 447)
<https://reviews.apache.org/r/57061/#comment239057>

    Nitpick: You are somewhat inconsistent with how you format log messages.



src/main/java/org/apache/aurora/scheduler/mesos/VersionedSchedulerDriverService.java (lines
91 - 94)
<https://reviews.apache.org/r/57061/#comment239060>

    The log messages are duplicated (see V1 scheduler methods). Can we drop them in one place?


- Stephan Erb


On Feb. 27, 2017, 10:44 p.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57061/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2017, 10:44 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Mehrdad Nurolahzade, and Stephan Erb.
> 
> 
> Bugs: AURORA-1887 and AURORA-1888
>     https://issues.apache.org/jira/browse/AURORA-1887
>     https://issues.apache.org/jira/browse/AURORA-1888
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This patch completes the design doc[1] and enables operators to choose between
> two V1 Mesos API implementations. The first is `V0Mesos` which offers the V1 API
> backed by the scheduler driver and the second is `V1Mesos` which offers the V1
> API backed by a new HTTP API implementation.
> 
> There are three sets of changes in this patch.
> 
> First, the V1 Mesos code requires a Scheduler callback with a different API. To
> maximize code reuse, `MesosSchedulerImpl` was extended to implement the new
> callback as well as the old callback. The code and tests were reshufled to
> maxmize logic reuse.
> 
> Second, a new driver implementation using the new API was created. All of the
> logic for the new driver is encapsulated in the
> `VersionedSchedulerDriverService` class.
> 
> Third, some wiring changes were done to allow for Guice to do it's work and
> allow for operators to select between the different driver implementations.
> 
> [1] https://docs.google.com/document/d/1bWK8ldaQSsRXvdKwTh8tyR_0qMxAlnMW70eOKoU3myo
> 
> 
> Diffs
> -----
> 
>   examples/vagrant/upstart/aurora-scheduler.conf 49fdcbd8b7406a59ae7882473b9eddbfce3ece7c

>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java e2ef9c30720698263106f22e3e24db5d0468b155

>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 805e9de9bc45396cb8fc6e33ddb3d7428312c608

>   src/main/java/org/apache/aurora/scheduler/mesos/LibMesosLoadingModule.java e1a23590c795a489e753b77b0835d30d3be174b5

>   src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java eb210962c54cd0d33e3760b32f5b0ca1a7079204

>   src/main/java/org/apache/aurora/scheduler/mesos/ProtosConversion.java bc9e23b7410c00b7d5ffa4f23db93a51e9d0f405

>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 5519323079b2c957a23e093dcc77929148b4a59a

>   src/main/java/org/apache/aurora/scheduler/mesos/VersionedDriverFactory.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/mesos/VersionedSchedulerDriverService.java
PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 05518048ca5518a007281269aa402a7d0710eb62

>   src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java c599fe30bc903b3a3fb178df70a46d2421b6c45e

>   src/test/java/org/apache/aurora/scheduler/mesos/VersionedSchedulerDriverServiceTest.java
PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java f2275c757ebfa52179e31b95bf0c02b6753fb7e3

> 
> Diff: https://reviews.apache.org/r/57061/diff/
> 
> 
> Testing
> -------
> 
> The e2e test has been run three times, each time with a different driver option.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>


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