feat: OkHttpEngine BYOC #1437
feat: OkHttpEngine BYOC #1437
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
Affected ArtifactsChanged in size
|
lauzadis
changed the title
executorService configuration to OkHttpEngine
| public constructor(client: OkHttpClient) : this(OkHttpEngineConfig.Default) { | ||
| this.manageClient = false | ||
|
|
||
| // destroy our client | ||
| this.client.apply { | ||
| connectionPool.evictAll() | ||
| dispatcher.executorService.shutdown() | ||
| } | ||
|
|
||
| // use the user-provided client, configured with metrics collection | ||
| this.client = client.newBuilder().apply { | ||
| eventListenerFactory { call -> | ||
| EventListenerChain( | ||
| listOf( | ||
| HttpEngineEventListener(client.connectionPool, config.hostResolver, client.dispatcher, metrics, call), | ||
| ), | ||
| ) | ||
| } | ||
| addInterceptor(MetricsInterceptor) | ||
| }.build() | ||
| } |
There was a problem hiding this comment.
Correctness: We shouldn't create an OkHttp client just to close it and replace it with the user's client. Let's refactor these constructors to avoid unnecessary client creation and to remove the need for var fields.
There was a problem hiding this comment.
refactored to initialize the client lazily. I think we still need one var since we can't edit the primary constructor
runtime/build.gradle.kts
Outdated
|
|
||
| // configureIosSimulatorTasks() | ||
| configureIosSimulatorTasks() | ||
|
|
There was a problem hiding this comment.
Question: Why did this change?
There was a problem hiding this comment.
I don't know why it was commented, but I don't think it should be. This is necessary for TLS tests to pass, it was originally added to aws-crt-kotlin and I applied it here too, but it might not be necessary since our CI has been passing without it. Want me to delete it?
There was a problem hiding this comment.
If it's not necessary for CI and we don't need it for any other reason then yes, I think we should remove it.
runtime/build.gradle.kts
Outdated
|
|
||
| // configureIosSimulatorTasks() | ||
| configureIosSimulatorTasks() | ||
|
|
There was a problem hiding this comment.
If it's not necessary for CI and we don't need it for any other reason then yes, I think we should remove it.
| private var userProvidedClient: OkHttpClient? = null | ||
| public constructor(client: OkHttpClient) : this(OkHttpEngineConfig.Default) { | ||
| userProvidedClient = client | ||
| } | ||
|
|
There was a problem hiding this comment.
Comment: The refactor looks much better and addresses my main concern about unnecessary client instantiation. Thanks!
Nit:
we can't edit the primary constructor
I don't believe a constructor's "primaryness" affects the public API. It's a source code term which indicates it's the one that appears on the class declaration line, as opposed to those which appear in the body. The compiler also enforces that if a primary constructor exists then all secondary constructors must invoke it.
I believe this code could be further improved by moving the existing primary constructor to a secondary one and either a) making each secondary constructor fully set the initial state or b) adding a new private primary constructor to set state. As I noted above, my primary concern is addressed so I'll leave it up to you whether to continue iterating.
There was a problem hiding this comment.
Refactored a bit more for option b), what do you think?
3 participants
Adds a new constructor to OkHttpEngine which allows users to bring their own client, allowing them to customize behavior outside of what the SDK exposes.
Issue #
Closes aws/aws-sdk-kotlin#1707
Description of changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.