You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Jordan Ly <jo...@gmail.com> on 2017/09/30 00:17:52 UTC
Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
Bugs: AURORA-1773
https://issues.apache.org/jira/browse/AURORA-1773
Repository: aurora
Description
-------
Hijacking https://reviews.apache.org/r/59703
From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
Diffs
-----
build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
Diff: https://reviews.apache.org/r/62700/diff/1/
Testing
-------
./gradlew test
Tested proper shutdown occurs in Vagrant.
Currently scale-testing over the weekend.
Thanks,
Jordan Ly
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review186767
-----------------------------------------------------------
Ship it!
Master (24d2caf) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Sept. 30, 2017, 12:17 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2017, 12:17 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/2/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
> Currently scale-testing over the weekend.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review186900
-----------------------------------------------------------
LGMT. Minor comments.
build.gradle
Line 85 (original), 85 (patched)
<https://reviews.apache.org/r/62700/#comment263785>
nit - Make it more specific to the library name - s/httpclientRev/asyncHttpclientRev/
build.gradle
Line 86 (original), 86 (patched)
<https://reviews.apache.org/r/62700/#comment263778>
Remove this unused.
src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java
Lines 74-76 (original), 75-77 (patched)
<https://reviews.apache.org/r/62700/#comment263820>
Wonder if we should configure other timeouts as well? Like `readTimeout` etc.
https://github.com/AsyncHttpClient/async-http-client/blob/master/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java#L81-L108
- Santhosh Kumar Shanmugham
On Sept. 29, 2017, 5:17 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2017, 5:17 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/2/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
> Currently scale-testing over the weekend.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review186926
-----------------------------------------------------------
Master (6fd6d50) is red with this patch.
./build-support/jenkins/build.sh
[68] ./~/react-dom/cjs/react-dom.development.js 607 kB {0} [built]
[79] ./~/react-router-dom/es/withRouter.js 143 bytes {0} [built]
[86] ./~/react/cjs/react.development.js 55.3 kB {0} [built]
[87] ./~/react/cjs/react.production.min.js 5.61 kB {0} [built]
+ 75 hidden modules
:processResources
:classes
:jar
:startScripts
:distTar
:distZip
:assemble
:compileTestJavaNote: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
:processTestResources
:testClasses
:compileJmhJavaNote: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
:processJmhResources NO-SOURCE
:jmhClasses
:checkstyleJmh
:checkstyleMain
:checkstyleTest[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java:34:8: Unused import - org.asynchttpclient.Response. [UnusedImports]
FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':checkstyleTest'.
> Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/test.html
* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
* Get more help at https://help.gradle.org
BUILD FAILED in 2m 56s
38 actionable tasks: 32 executed, 6 up-to-date
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Oct. 3, 2017, 4:25 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2017, 4:25 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/3/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review186987
-----------------------------------------------------------
Ship it!
Master (6fd6d50) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Oct. 3, 2017, 5:01 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2017, 5:01 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/4/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review187008
-----------------------------------------------------------
Ship it!
The code itself looks good to me +- the switch to a proper integration test.
- Stephan Erb
On Oct. 3, 2017, 7:01 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2017, 7:01 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/4/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Jordan Ly <jo...@gmail.com>.
> On Oct. 3, 2017, 5:08 p.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/events/Webhook.java
> > Line 87 (original), 102 (patched)
> > <https://reviews.apache.org/r/62700/diff/4/?file=1845143#file1845143line109>
> >
> > Do you know how the timeout scenario works here? Will a timeout result in `onThrowable` or `onCompleted`. How do we know that it was indeed a timeout in either case? Would we consider timeout a user error or system error?
I believe a timeout will be caught on the outer `try-catch` statement, resulting in a system error.
I can add a separate metric tracking timeouts specifically, looking for `java.util.concurrent.TimeoutException` which is thrown by the highlighted statement.
- Jordan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review186975
-----------------------------------------------------------
On Oct. 3, 2017, 5:01 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2017, 5:01 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/4/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
> On Oct. 3, 2017, 10:08 a.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/events/Webhook.java
> > Line 87 (original), 102 (patched)
> > <https://reviews.apache.org/r/62700/diff/4/?file=1845143#file1845143line109>
> >
> > Do you know how the timeout scenario works here? Will a timeout result in `onThrowable` or `onCompleted`. How do we know that it was indeed a timeout in either case? Would we consider timeout a user error or system error?
>
> Jordan Ly wrote:
> I believe a timeout will be caught on the outer `try-catch` statement, resulting in a system error.
>
> I can add a separate metric tracking timeouts specifically, looking for `java.util.concurrent.TimeoutException` which is thrown by the highlighted statement.
>
> Jordan Ly wrote:
> Actually, I lied -- it is caught by `onThrowable` and is a user-level error.
>
> I think this is the correct case and we already log the error -- I am not sure if there is any value in further granularity.
+1
- Santhosh Kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review186975
-----------------------------------------------------------
On Oct. 3, 2017, 10:01 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2017, 10:01 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/4/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review186975
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/events/Webhook.java
Line 87 (original), 102 (patched)
<https://reviews.apache.org/r/62700/#comment263925>
Do you know how the timeout scenario works here? Will a timeout result in `onThrowable` or `onCompleted`. How do we know that it was indeed a timeout in either case? Would we consider timeout a user error or system error?
- Santhosh Kumar Shanmugham
On Oct. 3, 2017, 10:01 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2017, 10:01 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/4/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review187034
-----------------------------------------------------------
Ship it!
Master (4e7cdc4) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Oct. 4, 2017, 12:16 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2017, 12:16 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle 069c62ecfc097a3ff22bcf207b2574dd417b0b15
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/main/resources/org/apache/aurora/scheduler/webhook.json b78c063d0a72d7a22a8806451e4002d27eaf759a
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/5/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review187031
-----------------------------------------------------------
src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java
Line 47 (original), 54 (patched)
<https://reviews.apache.org/r/62700/#comment263986>
I believe `extends EasyMockTest` is no longer needed.
src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java
Lines 112 (patched)
<https://reviews.apache.org/r/62700/#comment263985>
Slightly more work, but you'll want to use an ephemeral port to avoid conflict.
src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java
Lines 144 (patched)
<https://reviews.apache.org/r/62700/#comment263987>
This leaves much to be desired, since it's handling the union of all test cases. Can you instead start the jetty server in each test case, and wire in a very specific handler appropriate for the test?
- Bill Farner
On Oct. 3, 2017, 5:16 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2017, 5:16 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle 069c62ecfc097a3ff22bcf207b2574dd417b0b15
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/main/resources/org/apache/aurora/scheduler/webhook.json b78c063d0a72d7a22a8806451e4002d27eaf759a
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/5/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review187094
-----------------------------------------------------------
Ship it!
Nice!
- Bill Farner
On Oct. 3, 2017, 9:31 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2017, 9:31 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle 069c62ecfc097a3ff22bcf207b2574dd417b0b15
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/main/resources/org/apache/aurora/scheduler/webhook.json b78c063d0a72d7a22a8806451e4002d27eaf759a
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/7/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review187047
-----------------------------------------------------------
Master (4e7cdc4) is red with this patch.
./build-support/jenkins/build.sh
:processJmhResources NO-SOURCE
:jmhClasses
:checkstyleJmh
:checkstyleMain
:checkstyleTest
:findbugsJmh
:findbugsMain
:findbugsTest
:licenseJmh UP-TO-DATE
:licenseMain UP-TO-DATE
:licenseTest UP-TO-DATE
:license UP-TO-DATE
:pmdJmh
:pmdMain
:pmdTest
:test
org.apache.aurora.scheduler.events.WebhookTest > testTaskChangedWithOldStateSuccess FAILED
java.lang.AssertionError at WebhookTest.java:160
I1004 04:46:59.646 [ShutdownHook, SchedulerMain] Stopping scheduler services.
1144 tests completed, 1 failed, 2 skipped
:test FAILED
:jacocoTestReport
Coverage report generated: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/jacoco/test/html/index.html
:jacocoTestCoverageVerification
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':test'.
> There were failing tests. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/tests/test/index.html
* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
* Get more help at https://help.gradle.org
BUILD FAILED in 6m 12s
50 actionable tasks: 41 executed, 9 up-to-date
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Oct. 4, 2017, 4:31 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2017, 4:31 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle 069c62ecfc097a3ff22bcf207b2574dd417b0b15
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/main/resources/org/apache/aurora/scheduler/webhook.json b78c063d0a72d7a22a8806451e4002d27eaf759a
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/6/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review187049
-----------------------------------------------------------
Ship it!
Master (4e7cdc4) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Oct. 4, 2017, 4:31 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2017, 4:31 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle 069c62ecfc097a3ff22bcf207b2574dd417b0b15
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/main/resources/org/apache/aurora/scheduler/webhook.json b78c063d0a72d7a22a8806451e4002d27eaf759a
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/7/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Jordan Ly <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/
-----------------------------------------------------------
(Updated Oct. 4, 2017, 4:31 a.m.)
Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
Changes
-------
Additional test refactoring
Bugs: AURORA-1773
https://issues.apache.org/jira/browse/AURORA-1773
Repository: aurora
Description
-------
Hijacking https://reviews.apache.org/r/59703
From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
Diffs (updated)
-----
build.gradle 069c62ecfc097a3ff22bcf207b2574dd417b0b15
src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
src/main/resources/org/apache/aurora/scheduler/webhook.json b78c063d0a72d7a22a8806451e4002d27eaf759a
src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
Diff: https://reviews.apache.org/r/62700/diff/6/
Changes: https://reviews.apache.org/r/62700/diff/5-6/
Testing
-------
./gradlew test
Tested proper shutdown occurs in Vagrant.
Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
Thanks,
Jordan Ly
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Jordan Ly <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/
-----------------------------------------------------------
(Updated Oct. 4, 2017, 12:16 a.m.)
Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
Changes
-------
Better testing of Webhook/AsyncHttpClient.
Bugs: AURORA-1773
https://issues.apache.org/jira/browse/AURORA-1773
Repository: aurora
Description
-------
Hijacking https://reviews.apache.org/r/59703
From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
Diffs (updated)
-----
build.gradle 069c62ecfc097a3ff22bcf207b2574dd417b0b15
src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
src/main/resources/org/apache/aurora/scheduler/webhook.json b78c063d0a72d7a22a8806451e4002d27eaf759a
src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
Diff: https://reviews.apache.org/r/62700/diff/5/
Changes: https://reviews.apache.org/r/62700/diff/4-5/
Testing
-------
./gradlew test
Tested proper shutdown occurs in Vagrant.
Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
Thanks,
Jordan Ly
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review186994
-----------------------------------------------------------
Ship it!
Ship It!
- Santhosh Kumar Shanmugham
On Oct. 3, 2017, 10:01 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2017, 10:01 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/4/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Stephan Erb <se...@apache.org>.
> On Oct. 3, 2017, 10:38 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java
> > Line 86 (original), 91 (patched)
> > <https://reviews.apache.org/r/62700/diff/4/?file=1845146#file1845146line98>
> >
> > This line results in very close coupling of the test to the implementation. Just about any change to the implementation will need to be mirrored here (since ~every interaction with `AsyncHttpClient` is reflected in an expected mock call). It also leaves us with little evidence that the implementation actually works.
> >
> > While it will make for more of an integration test, i suggest you actually start a jetty server and validate incoming HTTP requests. This should address the above 2 issues.
Good observation. I second your suggestion.
- Stephan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review187002
-----------------------------------------------------------
On Oct. 3, 2017, 7:01 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2017, 7:01 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/4/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review187002
-----------------------------------------------------------
src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java
Line 86 (original), 91 (patched)
<https://reviews.apache.org/r/62700/#comment263953>
This line results in very close coupling of the test to the implementation. Just about any change to the implementation will need to be mirrored here (since ~every interaction with `AsyncHttpClient` is reflected in an expected mock call). It also leaves us with little evidence that the implementation actually works.
While it will make for more of an integration test, i suggest you actually start a jetty server and validate incoming HTTP requests. This should address the above 2 issues.
- Bill Farner
On Oct. 3, 2017, 10:01 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2017, 10:01 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/4/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
>
> Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Jordan Ly <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/
-----------------------------------------------------------
(Updated Oct. 3, 2017, 5:01 p.m.)
Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
Changes
-------
Remove unused import.
Bugs: AURORA-1773
https://issues.apache.org/jira/browse/AURORA-1773
Repository: aurora
Description
-------
Hijacking https://reviews.apache.org/r/59703
From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
Diffs (updated)
-----
build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
Diff: https://reviews.apache.org/r/62700/diff/4/
Changes: https://reviews.apache.org/r/62700/diff/3-4/
Testing
-------
./gradlew test
Tested proper shutdown occurs in Vagrant.
Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
Thanks,
Jordan Ly
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Jordan Ly <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/
-----------------------------------------------------------
(Updated Oct. 3, 2017, 4:25 a.m.)
Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
Changes
-------
Renamed `httpclientRev` to `asyncHttpclientRev`
Removed unused dependency
Added additional relevant timeout configuration
Added scale-test result
Moved status handling from `onComplete` to `onStatusReceieved`
Bugs: AURORA-1773
https://issues.apache.org/jira/browse/AURORA-1773
Repository: aurora
Description
-------
Hijacking https://reviews.apache.org/r/59703
From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
Diffs (updated)
-----
build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
Diff: https://reviews.apache.org/r/62700/diff/3/
Changes: https://reviews.apache.org/r/62700/diff/2-3/
Testing (updated)
-------
./gradlew test
Tested proper shutdown occurs in Vagrant.
Scale tested up to 2000 TASK_LOST events with the registered endpoint waiting 5-10 minutes to response -- does not seem to block scheduling.
Thanks,
Jordan Ly
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review186765
-----------------------------------------------------------
Ship it!
Ship It!
- David McLaughlin
On Sept. 30, 2017, 12:17 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2017, 12:17 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/2/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
> Currently scale-testing over the weekend.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62700: Convert Webhook to AbstractIdleService,
use async HTTP client
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62700/#review186764
-----------------------------------------------------------
Master (24d2caf) is red with this patch.
./build-support/jenkins/build.sh
[87] ./~/react/cjs/react.production.min.js 5.61 kB {0} [built]
+ 75 hidden modules
:processResources
:classes
:jar
:startScripts
:distTar
:distZip
:assemble
:compileTestJava/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java:38: Note: Wrote forwarder org.apache.aurora.scheduler.thrift.aop.MockDecoratedThriftForwarder
@Forward(AnnotatedAuroraAdmin.class)
^
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
:processTestResources
:testClasses
:compileJmhJavaNote: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
:processJmhResources NO-SOURCE
:jmhClasses
:checkstyleJmh
:checkstyleMain[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/events/Webhook.java:20: Wrong order for 'com.google.common.annotations.VisibleForTesting' import. Order should be: java, javax, scala, com, net, org. Each group should be separated by a single blank line. [ImportOrder]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/events/Webhook.java:27: Wrong order for 'org.apache.aurora.common.stats.StatsProvider' import. Order should be: java, javax, scala, com, net, org. Each group should be separated by a single blank line. [ImportOrder]
FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':checkstyleMain'.
> Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.html
* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
* Get more help at https://help.gradle.org
BUILD FAILED in 4m 2s
37 actionable tasks: 31 executed, 6 up-to-date
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Sept. 30, 2017, 12:17 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62700/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2017, 12:17 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Bill Farner.
>
>
> Bugs: AURORA-1773
> https://issues.apache.org/jira/browse/AURORA-1773
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hijacking https://reviews.apache.org/r/59703
>
> From the above review: "Current code uses a synchronous HTTP client, which can block the EventBus. Switch to an async HTTP client."
>
> Previously, we had an issue where the HTTP client would have a non-daemon thread which caused the Scheduler to fail to shutdown. I converted it into an AbstractIdleService and properly closed the client in the shutdown() method. Additionally, I made a small tweak to the original code where we ABORT any response receieved after the status since we don't care. We just use the response code for stats.
>
>
> Diffs
> -----
>
> build.gradle f9579a38de95ff9e11cf2e44980d01e103226389
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java 05f46a1946e062ac57cca828c094586f4c983f45
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java da22c218c5fbe6607552edbb4f8e52850d718851
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 1f10af71830386652d21961b733bd0927c5436a1
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 07f39fa487e0ebd2252568750d5f36ac200a96aa
>
>
> Diff: https://reviews.apache.org/r/62700/diff/1/
>
>
> Testing
> -------
>
> ./gradlew test
>
> Tested proper shutdown occurs in Vagrant.
> Currently scale-testing over the weekend.
>
>
> Thanks,
>
> Jordan Ly
>
>