-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: add throttling service #325
ENH: add throttling service #325
Conversation
Signed-off-by: qchea <qchea@amazon.com>
Signed-off-by: qchea <qchea@amazon.com>
Signed-off-by: qchea <qchea@amazon.com>
.../http-source/src/test/java/com/amazon/dataprepper/plugins/source/loghttp/HTTPSourceTest.java
Show resolved
Hide resolved
final LogHTTPService logHTTPService = new LogHTTPService(requestTimeoutInMillis, buffer); | ||
sb.annotatedService(logHTTPService); | ||
sb.annotatedService("/log/ingest", logHTTPService); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the second time this string is used. I would recommend defining a constant for "/log/ingest".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we eventually want this to be a configuration. It might be good to just make that change now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dlvenable Upon a second thought, allow customizing the URI path for log ingestion seems not adding value to the source plugin despite that FluentBit HTTP output plugin allows configuring custom URI path. We might even just replace this URI path with root path "/".
For now I will make it a constant and we could deal with this minor change in later cleanup PRs.
Signed-off-by: qchea <qchea@amazon.com>
Signed-off-by: qchea <qchea@amazon.com>
Signed-off-by: qchea <qchea@amazon.com>
try { | ||
testWebClient.execute(testRequestHeaders, testHttpData).aggregate().join(); | ||
} catch (CompletionException e) { | ||
assertThat(e.getCause()).isInstanceOf(ResponseTimeoutException.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have noted this in the last PR, but this assertThat
will not be called unless an exception is actually thrown.
Please change to:
CompletionException actualException = assertThrows(CompletionException.class, () -> testWebClient.execute(testRequestHeaders, testHttpData).aggregate().join());
assertThat(actualException.getCause()).isInstanceOf(ResponseTimeoutException.class);
Also, below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
Signed-off-by: qchea <qchea@amazon.com>
* FEAT: load throttling service and add test cases Signed-off-by: qchea <qchea@amazon.com> * REF: URI path Signed-off-by: qchea <qchea@amazon.com> * MAINT: comment string Signed-off-by: qchea <qchea@amazon.com> * MAINT: DEFAULT_LOG_INGEST_URI Signed-off-by: qchea <qchea@amazon.com> * MAINT: clear up request after throttling Signed-off-by: qchea <qchea@amazon.com> * STY: spotless Signed-off-by: qchea <qchea@amazon.com> * MAINT: assertThrows exception Signed-off-by: qchea <qchea@amazon.com>
Description
This PR
Issues Resolved
#323
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.