Refactor two-client usage. #333
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dblock
requested review from
reta,
Bukhtawar,
madhusudhankonda,
saratvemulapalli and
VachaShah
as code owners
reta
reviewed
Jan 17, 2023
java-client/src/main/java/org/opensearch/client/transport/aws/AwsSdk2Transport.java
Outdated
Show resolved
Hide resolved
reta
reviewed
Jan 17, 2023
java-client/src/main/java/org/opensearch/client/transport/aws/AwsSdk2Transport.java
Outdated
Show resolved
Hide resolved
|
It looks better @dblock comparing to 2 clients |
dblock
force-pushed
the
refactor-transport
branch
from
a6d4617 to
948399d
Compare
|
Made some updates, thanks @reta. Seeing intermittent |
reta
previously approved these changes
Jan 18, 2023
|
@dblock the |
Signed-off-by: dblock <dblock@amazon.com>
dblock
force-pushed
the
refactor-transport
branch
from
948399d to
ac5cc66
Compare
Signed-off-by: dblock <dblock@amazon.com>
It's just a double-close on cleanup, fixed. |
reta
reviewed
Jan 18, 2023
java-client/src/main/java/org/opensearch/client/transport/aws/AwsSdk2Transport.java
Show resolved
Hide resolved
Signed-off-by: dblock <dblock@amazon.com>
dblock
force-pushed
the
refactor-transport
branch
from
170d383 to
a4988a6
Compare
reta
previously approved these changes
Jan 18, 2023
reta
reviewed
Jan 18, 2023
| @@ -257,6 +207,8 @@ public <RequestT, ResponseT, ErrorT> ResponseT performRequest( | |||
| } catch (InterruptedException e) { | |||
| throw new IOException("HttpRequest was interrupted", e); | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
This else is probably redundant at this point, right?
There was a problem hiding this comment.
Java is not smart enough to evaluate all paths and conclude that this code can never be reached. If you remove it, there will be an error that the method must return a value, and I don't want a catch all else where we make an assumption on the type of connection instance.
|
@saratvemulapalli care to do 2PR? |
VachaShah
reviewed
Jan 21, 2023
| */ | ||
| public AwsSdk2Transport( | ||
| @Nonnull SdkAsyncHttpClient asyncHttpClient, | ||
| @CheckForNull SdkHttpClient httpClient, |
There was a problem hiding this comment.
Just a minor comment: Should we call this syncHttpClient?
| @Nonnull Region signingRegion, | ||
| @CheckForNull AwsSdk2TransportOptions options) { | ||
| this(httpClient, asyncHttpClient, host, "es", signingRegion, options); | ||
| this((SdkAutoCloseable) httpClient, host, signingServiceName, signingRegion, options); |
There was a problem hiding this comment.
Same here as well syncHttpClient?
Signed-off-by: dblock <dblock@amazon.com>
dblock
force-pushed
the
refactor-transport
branch
from
93631d7 to
baa0428
Compare
reta
approved these changes
Jan 23, 2023
VachaShah
approved these changes
Jan 23, 2023
I am late to the party! |
|
@dblock Should we backport this to 2.x? |
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Mar 2, 2023
* Refactor two-client usage. Signed-off-by: dblock <dblock@amazon.com> * Avoid double-closing the client on cleanup. Signed-off-by: dblock <dblock@amazon.com> * Refactor constructors to take sync/async explicitly. Signed-off-by: dblock <dblock@amazon.com> * Corrected comments and cleaned up variable naming. Signed-off-by: dblock <dblock@amazon.com> Signed-off-by: dblock <dblock@amazon.com> (cherry picked from commit a0ca15c) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
VachaShah
pushed a commit
that referenced
this pull request
Mar 2, 2023
* Refactor two-client usage. * Avoid double-closing the client on cleanup. * Refactor constructors to take sync/async explicitly. * Corrected comments and cleaned up variable naming. (cherry picked from commit a0ca15c) Signed-off-by: dblock <dblock@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Signed-off-by: dblock dblock@amazon.com
Description
I really dislike that this code uses two different clients like it does. Is this better? Should I go further and add a class that wraps either of
SdkAutoCloseableand implements theexecutemethod so we don't have to check the type other than when creating the wrapper?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.