DynamoDB configurations update #3864
Conversation
marcinczeczko
commented
Sep 5, 2019
marcinczeczko
commented
- Refactored configuration structure - broken down config into dynamodb, AWS, SDK & HTTP client groups
- Added config of TLS Managers for sync/async clients
- Added config of HTTP Proxy for async client
- Added support for SDK client interceptors
|
@gsmet - I updated the extension due to the new config options available in latest AWS SDK: proxy for async, tlsmanagers, interceptors. |
marcinczeczko
force-pushed
the
dynamodb-more-configs
branch
from
d297583 to
82c717e
Compare
| /** | ||
| * Enable HTTP proxy | ||
| */ | ||
| @ConfigItem(defaultValue = "false") |
There was a problem hiding this comment.
defaultValue is not needed, boolean config item default to false
There was a problem hiding this comment.
I left it to let the doc generator to show default value
| * AWS SDK specific configurations | ||
| */ | ||
| @ConfigGroup | ||
| public class SdkConfig { |
There was a problem hiding this comment.
I'm not entirely sure about this specific SDK config with the sdk namespace. I mean you are configuring the base elements of the clients so having them at the root of the config would make sense to me.
WDYT?
There was a problem hiding this comment.
Fixed. Additionally migrated to the config docs automatically generated.
marcinczeczko
force-pushed
the
dynamodb-more-configs
branch
2 times, most recently
from
7a2a808 to
d8b74e9
Compare
| |`aws-java-sdk-NettyEventLoop` | ||
| |The thread name prefix for threads created by this thread factory used by event loop group. The prefix will be appended with a number unique to the thread factory and a number unique to the thread. | ||
| |=== | ||
| include::{generated-dir}/config/quarkus-dynamodb.adoc[opts=optional, leveloffset=+1] |
There was a problem hiding this comment.
+1 on the usage of generated doc, I am liking this :-)
marcinczeczko
force-pushed
the
dynamodb-more-configs
branch
from
d8b74e9 to
f4ed348
Compare
|
btw, I opened quickstarts PR quarkusio/quarkus-quickstarts#282 to align with this update. |
marcinczeczko
force-pushed
the
dynamodb-more-configs
branch
from
f4ed348 to
6ff3691
Compare
gsmet
left a comment
gsmet
left a comment
There was a problem hiding this comment.
I spotted a typo and added a comment and a question. Can you have a look?
We're nearly there :).
| /** | ||
| * The maximum number of connections allowed in the connection pool. | ||
| * | ||
| * Each built HTTP client has it's own private connection pool. |
There was a problem hiding this comment.
| * Each built HTTP client has it's own private connection pool. | |
| * Each built HTTP client has its own private connection pool. |
| /** | ||
| * The amount of time to wait when acquiring a connection from the pool before giving up and timing out. | ||
| * | ||
| * Default is 10 seconds. |
There was a problem hiding this comment.
I wonder if we should make them hard defaults so that they are properly documented. WDYT?
There was a problem hiding this comment.
Make sense, but I'm wondering now if I should document it in here. It's basically because those default are buried deep in the SDK builders and took me some time to find them out. So I think I should rather remove them. WDYT ?
|
|
||
| /** | ||
| * Configure the maximum amount of time that a connection should be allowed to remain open while idle. | ||
| * |
There was a problem hiding this comment.
In straight Javadoc, you would need a <p>, wouldn't you? Or the doc output is OK?
There was a problem hiding this comment.
Yes I missed that. Fixed.
There was a problem hiding this comment.
I mean added paragraph to the next row. Basically the doc output when checking the generated HTML is fine
- Refactored configuration structure - broken down config into dynamodb, AWS, SDK & HTTP client groups - Added config of TLS Managers for sync/async clients - Added config of HTTP Proxy for async client - Added support for SDK client interceptors
gsmet
force-pushed
the
dynamodb-more-configs
branch
from
1303514 to
8648da0
Compare
|
@marcinczeczko I rebased and I added a couple of commits to improve auto-documentation. Can you have a look at them? I'll wait for your blessing before merging. |
|
Thanks for those updates, looks great. You have my blessing :) |
|
Perfect, thanks! Kudos for this work. I will merge this and check the quickstart update tomorrow. |
3 participants
