Dynamodb - URL connection http client support #4471

marcinczeczko · 2019-10-09T11:15:45Z

Implementation for #4406

cescoffier · 2019-10-10T06:24:36Z

.../amazon-dynamodb/runtime/src/main/java/io/quarkus/dynamodb/runtime/SyncHttpClientConfig.java

+public class SyncHttpClientConfig {
+
+    /**
+     * Type of the sync HTTP client implementation


You should mention the accepted values here (APACHE, URL)

cescoffier · 2019-10-10T06:26:04Z

.../amazon-dynamodb/runtime/src/main/java/io/quarkus/dynamodb/runtime/SyncHttpClientConfig.java

+    public ApacheHttpClientConfig apacheHttp;
+
+    @ConfigGroup
+    public static class ApacheHttpClientConfig {


If you don't use the APACHE client, how do you configure all these details? Proxy, TLS?

There are no configuration options for URL connection other than socket timeout and connection timeout. But it's fine, as the lightweight url conn as sync client is fine for lambdas. For other app models like container talking to the dynamodb the apache sync client can be used that enables configurations for more connectivity aspects

gsmet · 2019-10-11T07:49:30Z

Just a heads up that I plan to review this one.

gsmet · 2019-10-14T13:22:13Z

.../amazon-dynamodb/runtime/src/main/java/io/quarkus/dynamodb/runtime/SyncHttpClientConfig.java

+     * Type of the sync HTTP client implementation
+     *
+     * <p>
+     * Accepted values are `URL` or `APACHE`


In fact, you shouldn't :). They will be automatically added to the documentation as it's an enum. So you can get rid of it.

Thanks, I forgot about that :) Will fix.

gsmet

I suggested a few things. Open to discussion so don't hesitate to push back :).

gsmet · 2019-10-14T13:22:56Z

.../amazon-dynamodb/runtime/src/main/java/io/quarkus/dynamodb/runtime/SyncHttpClientConfig.java

+     * The maximum amount of time to establish a connection before timing out.
+     *
+     * <p>
+     * Used by `URL` and `APACHE` clients


I think I would drop all these (the last line of documentation).

gsmet · 2019-10-14T13:23:52Z

.../amazon-dynamodb/runtime/src/main/java/io/quarkus/dynamodb/runtime/SyncHttpClientConfig.java

+     * Apache HTTP client additional configuration
+     */
+    @ConfigItem(name = ConfigItem.PARENT)
+    public ApacheHttpClientConfig apacheHttp;


Wondering if it shouldn't be in an apache group instead of added to the parent? This way, it's clear it's Apache specific and you don't need any additional documentation.

I think it make sense. However, since I wanted to avoid duplications, the connectionTimeout & socketTimeout that are on the parent and apply to URL connection are also applies to Apache. Wondering if it worth to reflect that on the documentation somehow..

gsmet · 2019-10-14T13:24:29Z

.../amazon-dynamodb/runtime/src/main/java/io/quarkus/dynamodb/runtime/SyncHttpClientConfig.java

+         * The amount of time to wait when acquiring a connection from the pool before giving up and timing out.
+         * 
+         * <p>
+         * Used by `APACHE` client only


Then you could drop this. I will tweak the documentation with some sections and it will be pretty much OK I think.

will drop it

gsmet · 2019-10-14T13:25:35Z

extensions/amazon-dynamodb/deployment/src/test/resources/sync-apache-broken-config.properties

@@ -1,3 +1,4 @@
+quarkus.dynamodb.sync-client.type = APACHE


So for enums, the recommended way is to use lower case, dashed versions in the application.properties. They will be automatically converted. Typically the documentation will contain apache and url so let's be consistent.

will clean it up

gsmet · 2019-10-14T13:26:27Z

bom/runtime/pom.xml

+                <groupId>software.amazon.awssdk</groupId>
+                <artifactId>url-connection-client</artifactId>
+                <version>${awssdk.version}</version>
+            </dependency>


What annoys me a bit is that you get all the dependencies with the extension. I wonder if it's a wise choice and we shouldn't be more careful about that especially in the lambda use case.

That being said, I'm not totally sure we can find a nice way to do that properly but IMHO it's worth a try.

If we can get to the point of the dependencies being optional and we can throw a warning or an error if something is configured for a "backend" and the correct dependencies are not in the classpath, that would be OK IMHO.

I don't think it's blocking for this PR tbh so I think we could merge this and see how we can improve things later.

I like the idea and in fact tried it, check the latest commits. In general:

I made client deps as optional

the deployment processor checks if required service exists on the classpath, if not throws deploymentException

the application should explicitly add required dependency to the classpath

Do you think it make sense ? Once you verify the idea, I'll update docs & quickstart

marcinczeczko

Please find the latest changes

gsmet · 2019-10-21T10:56:14Z

@marcinczeczko looks good to me. Can you rebase, squash everything and work on the doc/quickstart update?

I'll move that to 0.27.0 for now as I'm freezing the 0.26.0 code very soon.

marcinczeczko · 2019-10-21T12:05:56Z

@marcinczeczko looks good to me. Can you rebase, squash everything and work on the doc/quickstart update?

I'll move that to 0.27.0 for now as I'm freezing the 0.26.0 code very soon.

Thanks. I updated guide & quickstart (quarkusio/quarkus-quickstarts#328)

gsmet · 2019-10-28T16:22:46Z

I force-pushed a small change to fix a doc issue and rebased, I'll merge this one as soon as CI is green.

gsmet · 2019-10-28T20:40:37Z

CI failures is unrelated.

Comments addressed.

marcinczeczko · 2019-10-29T08:56:10Z

@gsmet - thanks !. By the way, I was working on another iteration of quarkified AWS SDK that spans across more AWS clients not only DynamoDB- #4968. It's WIP at the moment, but I'd like to validate if this is something you might be interested so I can continue on this. Thanks

marcinczeczko mentioned this pull request Oct 9, 2019

Dynamodb: Add support for Jdk buitin HttpClient - UrlConnectionHttpClient #4406

Closed

gsmet added this to the 0.25.0 milestone Oct 9, 2019

cescoffier previously requested changes Oct 10, 2019

View reviewed changes

marcinczeczko force-pushed the dynamodb-urlconnection-sync-client branch from d8973fc to 85344c5 Compare October 10, 2019 06:40

gsmet self-assigned this Oct 11, 2019

gsmet reviewed Oct 14, 2019

View reviewed changes

gsmet requested changes Oct 14, 2019

View reviewed changes

cescoffier modified the milestones: 0.25.0, 0.26.0 Oct 15, 2019

marcinczeczko force-pushed the dynamodb-urlconnection-sync-client branch from 85344c5 to 5d68871 Compare October 17, 2019 07:53

marcinczeczko commented Oct 17, 2019

View reviewed changes

gsmet modified the milestones: 0.26.0, 0.27.0 Oct 21, 2019

marcinczeczko mentioned this pull request Oct 21, 2019

Update dynamodb quickstart to support different http client implement… quarkusio/quarkus-quickstarts#328

Merged

marcinczeczko force-pushed the dynamodb-urlconnection-sync-client branch from ff96848 to a111da7 Compare October 21, 2019 12:03

marcinczeczko force-pushed the dynamodb-urlconnection-sync-client branch from a111da7 to 7212d70 Compare October 23, 2019 13:45

Dynamodb - URL connection http client support

f1a34cd

gsmet force-pushed the dynamodb-urlconnection-sync-client branch from 7212d70 to f1a34cd Compare October 28, 2019 16:22

gsmet approved these changes Oct 28, 2019

View reviewed changes

gsmet merged commit 46a3a3d into quarkusio:master Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamodb - URL connection http client support #4471

Dynamodb - URL connection http client support #4471

marcinczeczko commented Oct 9, 2019

cescoffier Oct 10, 2019

marcinczeczko Oct 10, 2019

cescoffier Oct 10, 2019

marcinczeczko Oct 10, 2019

gsmet commented Oct 11, 2019

gsmet Oct 14, 2019

marcinczeczko Oct 17, 2019

gsmet left a comment

gsmet Oct 14, 2019

marcinczeczko Oct 17, 2019

gsmet Oct 14, 2019

marcinczeczko Oct 17, 2019

gsmet Oct 14, 2019

marcinczeczko Oct 17, 2019

gsmet Oct 14, 2019

marcinczeczko Oct 17, 2019

gsmet Oct 14, 2019

marcinczeczko Oct 17, 2019

marcinczeczko left a comment

gsmet commented Oct 21, 2019

marcinczeczko commented Oct 21, 2019

gsmet commented Oct 28, 2019

gsmet commented Oct 28, 2019

marcinczeczko commented Oct 29, 2019

Dynamodb - URL connection http client support #4471

Dynamodb - URL connection http client support #4471

Conversation

marcinczeczko commented Oct 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet commented Oct 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcinczeczko left a comment

Choose a reason for hiding this comment

gsmet commented Oct 21, 2019

marcinczeczko commented Oct 21, 2019

gsmet commented Oct 28, 2019

gsmet commented Oct 28, 2019

marcinczeczko commented Oct 29, 2019