aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Shirchenko <cald...@gmail.com>
Subject Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
Date Thu, 19 May 2016 02:16:39 GMT


> On May 18, 2016, 10 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java, line 45
> > <https://reviews.apache.org/r/47440/diff/4/?file=1387379#file1387379line45>
> >
> >     I am surprised to find this one here. Are you planning to bundle a default config
file?

This is mainly for tests and as a sample to start with. If no user specified file is present
this isn't used.


> On May 18, 2016, 10 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 83
> > <https://reviews.apache.org/r/47440/diff/4/?file=1387377#file1387377line83>
> >
> >     What's that line doing?

Done. Fixing with recommended approach in https://docs.oracle.com/javase/7/docs/technotes/guides/net/http-keepalive.html


> On May 18, 2016, 10 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 75
> > <https://reviews.apache.org/r/47440/diff/4/?file=1387377#file1387377line75>
> >
> >     Is that an outdated comment?

It is. Removing. Thanks.


> On May 18, 2016, 10 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 39
> > <https://reviews.apache.org/r/47440/diff/4/?file=1387377#file1387377line39>
> >
> >     Thinking out loud: Do we have to worry about synchronization? Or is there a
guarantee that only one event will be dispatched at a time?
> >     
> >     If I understand this https://docs.oracle.com/javase/7/docs/technotes/guides/net/http-keepalive.html
and that https://stackoverflow.com/questions/16256681/how-to-reuse-httpurlconnection?lq=1
correctly, we could simply re-create a connection object each time. Java should do the right
thing behind the scenes. This would relieve us from any synchronization needs.

Yea, that's my understanding as well ie we don't have to worry about synchronization since
HttpURLConnection is recreated each time taskChangedState is triggered.


> On May 18, 2016, 10 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java, line 163
> > <https://reviews.apache.org/r/47440/diff/4/?file=1387376#file1387376line163>
> >
> >     By using the object as-is, we have effectively made in internal API public.
That could prevent us from doing necessary changes in the future, as we would have to adhere
to a deprecation cycle. It might therefore make sense to establish an explicit schema.

Good point. Essentially these are IScheduledTask objects which I believe already require a
deprecation cycle. Is this correct?


- Dmitriy


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


On May 18, 2016, 9:12 p.m., Dmitriy Shirchenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47440/
> -----------------------------------------------------------
> 
> (Updated May 18, 2016, 9:12 p.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
> 
>


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