Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add S3/MinIO support for application settings #2733
Add S3/MinIO support for application settings #2733
Changes from 39 commits
6f82ee8
b433a07
017c543
b93a281
c163511
a17952c
d1c6770
c2be492
58f47cb
5520b54
6ab79c9
183107a
9ad80b5
c9dd737
f02c424
79c2f94
75c7ba7
dc0f985
a18a9b7
67f047c
5ec3976
c958771
a3e350f
a0aa64a
53ce17b
5939670
a085ae6
62f5096
8132c26
754e318
1e40c25
20cdf45
99f499c
d089b27
f55beff
bdc37f2
7702b2c
a4469e2
87466a7
9c2b22c
6a5d1fa
6a61e58
70be3ca
c97e0bf
7ea3745
8655f55
5b21d38
f898648
e89c0f0
1229372
31febb4
cc29721
ea36f03
8f6cddd
cda0e5a
332d5d4
68f2975
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Hi. There are some things that need to be addressed:
Timeout
objectgetStoredData
implementation (currently it works sequentially: storedReq THEN storedImp, but you need to download it simultaneously)getMissingStoredDataIds
method (in terms of performance)Below you can find general idea:
Timeout
: timer created withvertx.setTimer
is reused for multipleFuture
s, which is preferable due to less jvm pollution.getStoredData
: storedReqs & storedImps are now downloaded simultaneously.getMissingStoredDataIds
: used much faster SetView implementationOptional
s removed to make code easier to read ( At least it seems to me that it has become “cleaner” :) )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.
Hi @CTMBNara
Thanks for the detailed feedback and the given code example 😃
I would highly argue against this for a couple of reasons
JdbcApplicationSettings
do not have this either, so adding it here would open too paths on how timeouts should be handledThat's probably a good thing to do, but the effect will probably be minimal as everything is cached and there are merely any requests be sent out.
Still I'm trying to implement this.
Just added this. Thanks for the code 👍
In the end I comply with the standards you set for this project, but I never looked back at
null
after switching from Java to Scala 10 years ago. My first contributions to prebid-server-java where bug fixes for null pointer exceptions. I understand thatOptional
in java is a bit more cumbersome to write, but in my experience it's a lot easier to maintain.If you want
null
you getnull
😂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.
Regarding the first one: In JdbcApplicationSettings timeout is passed to BasicJdbcClient which actually utilizes it.
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.
Regarding the second one: why do sequentially things that can be done in parallel? Its fairly easy change and can give us result much faster. Thus reduce amount of live requests and their objects that live in ram and put pressure on gc.
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.
Regarding the third one: this is not applicable for prebid server, we aim to support high thoughput and low latency, so we need to adjust timeouts on per-request basis to ensure that we fulfill SLA.
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.
Exactly my point. The
JdbcClient
is configured. The same should here be the case. We configure theS3Client
. Not the application settings implementation.From my experience parallel doesn't necessary equals faster. Number of cores, available RAM and number of Futures on the stack determine the performance. Still I copied your code example and things should now run in parallel.
That's totally fine. Then we should do it the same way as the JdbcSettings do it. Build a wrapper around the S3Client and use this instead. I don't feel competent enough to do this, though. I have no Vert.x experience and no idea on how to test this.
Is this something that can be added in a follow up PR?
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.
There is one more important point to explore. :)
In the above implementation, we don't tell the
CompletableFuture
s (from the S3AsyncClient) that the timeout has expired and their result is no longer needed. Perhaps we should notify them, but to do that we need to take a deep dive into the AWS library and see if notifying thefuture
s makes more than just conceptual sense.Guys, what do you think? @muuki88 @SerhiiNahornyi @Net-burst @And1sS @AntoxaAntoxic
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.
Code example:
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.
The AWS SDK 2.0 seems to have a way to plugin your own HTTP implementation.
https://github.com/aws/aws-sdk-java-v2/#using-the-sdk
There are a bunch of pre defined integrations: https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/http-configuration.html
This is something that we probably should flesh out - how to configure the http client
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.
Found this library, but it's rather small and unofficial: https://github.com/reactiverse/aws-sdk
It provides a vertx http client implementation for the aws sdk. What do you guys think?