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 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
Date Fri, 27 May 2016 13:53:23 GMT


> On May 22, 2016, 3:28 p.m., Stephan Erb wrote:
> > docs/reference/scheduler-configuration.md, line 222
> > <https://reviews.apache.org/r/47440/diff/5/?file=1387598#file1387598line222>
> >
> >     How about something like: "File to configure a HTTP webhook to receive task
state change events."

Done.


> On May 22, 2016, 3:28 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 46
> > <https://reviews.apache.org/r/47440/diff/5/?file=1387602#file1387602line46>
> >
> >     I'd favor if this method returns the `HttpURLConnection` rather than using a
mutable class member.

Done.


> On May 22, 2016, 3:28 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 59
> > <https://reviews.apache.org/r/47440/diff/5/?file=1387602#file1387602line59>
> >
> >     Is this some Kafka specific header? It does not seem to be default HTTP header
(https://en.wikipedia.org/wiki/List_of_HTTP_header_fields)
> >     
> >     Before we invent custom ones, I think we should rather stick the timestamp into
json playload.

Would be great if you could comment or clarify here.


> On May 22, 2016, 3:28 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 60
> > <https://reviews.apache.org/r/47440/diff/5/?file=1387602#file1387602line60>
> >
> >     Reading  https://docs.oracle.com/javase/6/docs/technotes/guides/net/http-keepalive.html
I am not sure if this is necessary:
> >     
> >     "The support for HTTP keep-Alive is done transparently. However, it can be controlled
by system properties http.keepAlive, and http.maxConnections, as well as by HTTP/1.1 specified
request and response headers."
> >     
> >     In anycase, shouldn't that be `keep-alive`? AFAIK only the head field name is
case-insensitive, but not the value.

Done.


> On May 22, 2016, 3:28 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java, line 42
> > <https://reviews.apache.org/r/47440/diff/5/?file=1387603#file1387603line42>
> >
> >     `requireNonNull` returns the input object if it is non-null. You can therefore
place it on the same line as your assignment.

Done.


> On May 22, 2016, 3:28 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java, line 45
> > <https://reviews.apache.org/r/47440/diff/5/?file=1387604#file1387604line45>
> >
> >     I'd vote to drop the file here and rather move it to test and/or documentation.

Would be great if you could do that. Shipping such a specific file in our normal source and
binary distribution does not make much sense.


> On May 22, 2016, 3:28 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java, line 73
> > <https://reviews.apache.org/r/47440/diff/5/?file=1387604#file1387604line73>
> >
> >     Maxim, is this the correct way to do it? Don't we normally throw in a `bind(XXX.class).in(Singleton.class)`?
 Or is this not necessary here because we are using `toInstance`?
> 
> Maxim Khutornenko wrote:
>     This should rather be:
>     ```
>     PubsubEventModule.bindSubscriber(binder(), Webhook.class);
>     bind(Webhook.class).in(Singleton.class);
>     ```
>     
>     The `WebhookInfo` should be injected into `Webhook` instead.

Done (sans the part with injecting the WebhookInfo, but I am kind of fine with that)


> On May 22, 2016, 3:28 p.m., Stephan Erb wrote:
> > src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java, line 28
> > <https://reviews.apache.org/r/47440/diff/5/?file=1387606#file1387606line28>
> >
> >     I have the impression that both `testTaskChangedState` and `testCallEndpoint`
are not properly exercicing the actual webhook class. Maxim, what do you think?
> >     
> >     Maybe an e2e test would be the best course of action here.

Still open, IMHO. Comments welcome.


- Stephan


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


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

>   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