aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Maxim Khutornenko <ma...@apache.org>
Subject Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
Date Wed, 25 May 2016 19:54:04 GMT

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



LGTM. Only minor comments/questions left.


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

    This initialization can be inlined now.



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

    <p>



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

    Don't you want to expose this all the way through to json? Fine if you don't but could
be handy if you want to tune it one day.
    
    Also, we generally put 'static final' fields first.



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

    Should be 4 spaces offset followed by empty line after '{'



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

    You don't have to wrap with `requireNonNull` here as `copyOf` already throws on null.



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

    I'd expect connection timeout to be here as well.


- Maxim Khutornenko


On May 25, 2016, 1:02 a.m., Dmitriy Shirchenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47440/
> -----------------------------------------------------------
> 
> (Updated May 25, 2016, 1:02 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Stephan Erb.
> 
> 
> 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
> 
>


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