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




docs/reference/scheduler-configuration.md (line 84)
<https://reviews.apache.org/r/47440/#comment198085>

    Formatting is off here and below.



src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 30)
<https://reviews.apache.org/r/47440/#comment198062>

    Missing javadoc for class and public methods.



src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 45)
<https://reviews.apache.org/r/47440/#comment198072>

    +1
    
    Also, consider caching and reusing the connection in a `LoadingCache` [1] 
with `expireAfterAccess` set to close it due to inactivity. You can 
implementing disconnect() inside a `RemovalListener`.
    
    [1] - https://github.com/google/guava/wiki/CachesExplained



src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 57)
<https://reviews.apache.org/r/47440/#comment198080>

    Consider this as fine or debug message to avoid polluting logs.



src/main/java/org/apache/aurora/scheduler/events/Webhook.java (lines 60 - 63)
<https://reviews.apache.org/r/47440/#comment198065>

    Wrap into try-with-resources?



src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 73)
<https://reviews.apache.org/r/47440/#comment198082>

    This should be LOG.error().



src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 76)
<https://reviews.apache.org/r/47440/#comment198083>

    +1, see my suggestion above.



src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 80)
<https://reviews.apache.org/r/47440/#comment198079>

    This should be fine or debug instead.



src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java (line 24)
<https://reviews.apache.org/r/47440/#comment198075>

    s/public/private here and below in favor of public getters.



src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java (line 29)
<https://reviews.apache.org/r/47440/#comment198061>

    Offset is off here.



src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java (line 31)
<https://reviews.apache.org/r/47440/#comment198060>

    move to previous line



src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java (lines 32 - 
33)
<https://reviews.apache.org/r/47440/#comment198076>

    requireNonNull
    
    Also, wrap headers assignment with ImmutableMap.copyOf().



src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java (lines 39 - 
41)
<https://reviews.apache.org/r/47440/#comment198077>

    formatting is off



src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java (line 46)
<https://reviews.apache.org/r/47440/#comment198050>

    Feels a bit redundant given the WEBHOOK_CONFIG_FILE. Have you considered 
controlling this feature by the config file presence?



src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java (line 57)
<https://reviews.apache.org/r/47440/#comment198051>

    This constructor is not test-only.



src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java (line 63)
<https://reviews.apache.org/r/47440/#comment198052>

    s/public//
    
    Also, if you take my suggestion above this constructor should take file 
rather than flag arg.



src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java (line 77)
<https://reviews.apache.org/r/47440/#comment198053>

    @VisibleForTesting



src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java (line 91)
<https://reviews.apache.org/r/47440/#comment198059>

    This approach of reading config files looks pretty much identical to the 
one in TierModule. How about moving it into a new util class (e.g.: ConfigUtil 
alongside GuiceUtil) to reduce repetition?



src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java (line 100)
<https://reviews.apache.org/r/47440/#comment198055>

    remove


- Maxim Khutornenko


On May 17, 2016, 6:44 a.m., Dmitriy Shirchenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47440/
> -----------------------------------------------------------
> 
> (Updated May 17, 2016, 6:44 a.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Bugs: AURORA-1683
>     https://issues.apache.org/jira/browse/AURORA-1683
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Looking for some feedback whether I'm on the correct path in adding a 
> webhook. All comments are welcome!
> 
> 
> Diffs
> -----
> 
>   docs/reference/scheduler-configuration.md 
> f7d676d0ed6bc536f4341dbb9365cf50e8607efb 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 3d9e706de564df5e24cb34265bebc0db1cad11a0 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 9ebfe230836e88a97bc60092373f72f176a8f6f2 
>   src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 
> 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc 
>   src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47440/diff/
> 
> 
> Testing
> -------
> 
> Need to fix tests.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Reply via email to