Skip to content

Remove the use of S3NativeClient and integrate with S3Client #2976

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

Merged
merged 9 commits into from
Jan 24, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import software.amazon.awssdk.annotations.SdkProtectedApi;
import software.amazon.awssdk.core.interceptor.trait.HttpChecksumRequired;
import software.amazon.awssdk.http.SdkHttpExecutionAttributes;

/**
* Attributes that can be applied to all sdk requests. Only generated code from the SDK clients should set these values.
Expand Down Expand Up @@ -47,6 +48,12 @@ public final class SdkInternalExecutionAttribute extends SdkExecutionAttribute {
public static final ExecutionAttribute<Boolean> DISABLE_HOST_PREFIX_INJECTION =
new ExecutionAttribute<>("DisableHostPrefixInjection");

/**
* The SDK HTTP attributes that can be passed to the HTTP client
*/
public static final ExecutionAttribute<SdkHttpExecutionAttributes> SDK_HTTP_EXECUTION_ATTRIBUTES =
new ExecutionAttribute<>("SdkHttpExecutionAttributes");

private SdkInternalExecutionAttribute() {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

package software.amazon.awssdk.core.internal.http.pipeline.stages;

import static software.amazon.awssdk.core.interceptor.SdkInternalExecutionAttribute.SDK_HTTP_EXECUTION_ATTRIBUTES;
import static software.amazon.awssdk.core.internal.http.timers.TimerUtils.resolveTimeoutInMillis;
import static software.amazon.awssdk.http.Header.CONTENT_LENGTH;

Expand Down Expand Up @@ -180,15 +181,19 @@ private CompletableFuture<Response<OutputT>> executeHttpRequest(SdkHttpFullReque

MetricCollector httpMetricCollector = MetricUtils.createHttpMetricsCollector(context);

AsyncExecuteRequest executeRequest = AsyncExecuteRequest.builder()
AsyncExecuteRequest.Builder executeRequestBuilder = AsyncExecuteRequest.builder()
.request(requestWithContentLength)
.requestContentPublisher(requestProvider)
.responseHandler(wrappedResponseHandler)
.fullDuplex(isFullDuplex(context.executionAttributes()))
.metricCollector(httpMetricCollector)
.build();
.metricCollector(httpMetricCollector);
if (context.executionAttributes().getAttribute(SDK_HTTP_EXECUTION_ATTRIBUTES) != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a variant of getAttribute() that returns an Optional, so we don't have to do multiple lookups? It's a hyper-nit in terms of performance, but it would also help readability in places like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it'd be nice to have another method returning optional but since overload is not possible, I'd be hesitant to introduce another method with a different name unless we have strong reasons.

executeRequestBuilder.httpExecutionAttributes(
context.executionAttributes()
.getAttribute(SDK_HTTP_EXECUTION_ATTRIBUTES));
Comment on lines +191 to +193
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we just copied all executionAttributes to the request, rather than selectively choosing which ones to copy? Are we worried that would leak abstractions that are not relevant to the request?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, most of the execution attributes are used in the SDK core and not relevant to the HTTP request.

}

CompletableFuture<Void> httpClientFuture = doExecuteHttpRequest(context, executeRequest);
CompletableFuture<Void> httpClientFuture = doExecuteHttpRequest(context, executeRequestBuilder.build());

TimeoutTracker timeoutTracker = setupAttemptTimer(responseFuture, context);
context.apiCallAttemptTimeoutTracker(timeoutTracker);
Expand Down
5 changes: 5 additions & 0 deletions http-client-spi/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@
<artifactId>reactive-streams-tck</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>nl.jqno.equalsverifier</groupId>
<artifactId>equalsverifier</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

package software.amazon.awssdk.http;

import software.amazon.awssdk.annotations.SdkPublicApi;
import software.amazon.awssdk.http.async.AsyncExecuteRequest;
import software.amazon.awssdk.utils.AttributeMap;

/**
* An attribute attached to a particular HTTP request execution, stored in {@link SdkHttpExecutionAttributes}. It can be
* configured on an {@link AsyncExecuteRequest} via
* {@link AsyncExecuteRequest.Builder#putHttpExecutionAttribute(SdkHttpExecutionAttribute,
* Object)}
*
* @param <T> The type of data associated with this attribute.
*/
@SdkPublicApi
public abstract class SdkHttpExecutionAttribute<T> extends AttributeMap.Key<T> {

protected SdkHttpExecutionAttribute(Class<T> valueType) {
super(valueType);
}

protected SdkHttpExecutionAttribute(UnsafeValueType unsafeValueType) {
super(unsafeValueType);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

package software.amazon.awssdk.http;

import java.util.Map;
import java.util.Objects;
import software.amazon.awssdk.annotations.SdkPublicApi;
import software.amazon.awssdk.http.async.AsyncExecuteRequest;
import software.amazon.awssdk.utils.AttributeMap;
import software.amazon.awssdk.utils.Validate;
import software.amazon.awssdk.utils.builder.CopyableBuilder;
import software.amazon.awssdk.utils.builder.ToCopyableBuilder;

/**
* An immutable collection of {@link SdkHttpExecutionAttribute}s that can be configured on an {@link AsyncExecuteRequest} via
* {@link AsyncExecuteRequest.Builder#httpExecutionAttributes(SdkHttpExecutionAttributes)}
*/
@SdkPublicApi
public final class SdkHttpExecutionAttributes implements ToCopyableBuilder<SdkHttpExecutionAttributes.Builder,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need Sdk here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I added Sdk prefix because the client name (SdkAsyncHttpClient) has it as well

SdkHttpExecutionAttributes> {
private final AttributeMap attributes;

private SdkHttpExecutionAttributes(Builder builder) {
this.attributes = builder.sdkHttpExecutionAttributes.build();
}

/**
* Retrieve the current value of the provided attribute in this collection of attributes. This will return null if the value
* is not set.
*/
public <T> T attribute(SdkHttpExecutionAttribute<T> attribute) {
return attributes.get(attribute);
}

public static Builder builder() {
return new Builder();
}

@Override
public Builder toBuilder() {
return new Builder(attributes);
}

public SdkHttpExecutionAttributes copy() {
return toBuilder().build();
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}

if (o == null || getClass() != o.getClass()) {
return false;
}

SdkHttpExecutionAttributes that = (SdkHttpExecutionAttributes) o;

return Objects.equals(attributes, that.attributes);
}

@Override
public int hashCode() {
return attributes.hashCode();
}

public static final class Builder implements CopyableBuilder<SdkHttpExecutionAttributes.Builder, SdkHttpExecutionAttributes> {
private AttributeMap.Builder sdkHttpExecutionAttributes = AttributeMap.builder();

private Builder(AttributeMap attributes) {
sdkHttpExecutionAttributes = attributes.toBuilder();
}

private Builder() {
}

/**
* Add a mapping between the provided key and value.
*/
public <T> SdkHttpExecutionAttributes.Builder put(SdkHttpExecutionAttribute<T> key, T value) {
Validate.notNull(key, "Key to set must not be null.");
sdkHttpExecutionAttributes.put(key, value);
return this;
}

/**
* Adds all the attributes from the map provided.
*/
public SdkHttpExecutionAttributes.Builder putAll(Map<? extends SdkHttpExecutionAttribute<?>, ?> attributes) {
sdkHttpExecutionAttributes.putAll(attributes);
return this;
}

@Override
public SdkHttpExecutionAttributes build() {
return new SdkHttpExecutionAttributes(this);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@

import java.util.Optional;
import software.amazon.awssdk.annotations.SdkPublicApi;
import software.amazon.awssdk.http.SdkHttpExecutionAttribute;
import software.amazon.awssdk.http.SdkHttpExecutionAttributes;
import software.amazon.awssdk.http.SdkHttpRequest;
import software.amazon.awssdk.metrics.MetricCollector;
import software.amazon.awssdk.utils.Validate;

/**
* Request object containing the parameters necessary to make an asynchronous HTTP request.
Expand All @@ -32,13 +35,15 @@ public final class AsyncExecuteRequest {
private final SdkAsyncHttpResponseHandler responseHandler;
private final MetricCollector metricCollector;
private final boolean isFullDuplex;
private final SdkHttpExecutionAttributes sdkHttpExecutionAttributes;

private AsyncExecuteRequest(BuilderImpl builder) {
this.request = builder.request;
this.requestContentPublisher = builder.requestContentPublisher;
this.responseHandler = builder.responseHandler;
this.metricCollector = builder.metricCollector;
this.isFullDuplex = builder.isFullDuplex;
this.sdkHttpExecutionAttributes = builder.executionAttributesBuilder.build();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be referencing a builder or an already-built object?

Copy link
Contributor Author

@zoewangg zoewangg Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? Is there any benefit of referencing an already-built object?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems unusual to me that we're calling builder.executionAttributesBuilder.build() rather than just builder.executionAttributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, executionAttributes is immutable, so we have to keep a executionAttributesBuilder in the Builder inner class to allow users to provide a single attribute. We could add another executionAttributes field which will be built in the AsyncExecuteRequest build() method, but I feel it's a bit overkill.

}

/**
Expand Down Expand Up @@ -76,6 +81,13 @@ public boolean fullDuplex() {
return isFullDuplex;
}

/**
* @return the SDK HTTP execution attributes associated with this request
*/
public SdkHttpExecutionAttributes httpExecutionAttributes() {
return sdkHttpExecutionAttributes;
}

public static Builder builder() {
return new BuilderImpl();
}
Expand Down Expand Up @@ -125,6 +137,25 @@ public interface Builder {
*/
Builder fullDuplex(boolean fullDuplex);

/**
* Put an HTTP execution attribute into to the collection of HTTP execution attributes for this request
*
* @param attribute The execution attribute object
* @param value The value of the execution attribute.
*/
<T> Builder putHttpExecutionAttribute(SdkHttpExecutionAttribute<T> attribute, T value);

/**
* Sets the additional HTTP execution attributes collection for this request.
* <p>
* This will override the attributes configured through
* {@link #putHttpExecutionAttribute(SdkHttpExecutionAttribute, Object)}
*
* @param executionAttributes Execution attributes map for this request.
* @return This object for method chaining.
*/
Builder httpExecutionAttributes(SdkHttpExecutionAttributes executionAttributes);

AsyncExecuteRequest build();
}

Expand All @@ -134,6 +165,7 @@ private static class BuilderImpl implements Builder {
private SdkAsyncHttpResponseHandler responseHandler;
private MetricCollector metricCollector;
private boolean isFullDuplex;
private SdkHttpExecutionAttributes.Builder executionAttributesBuilder = SdkHttpExecutionAttributes.builder();

@Override
public Builder request(SdkHttpRequest request) {
Expand Down Expand Up @@ -165,6 +197,20 @@ public Builder fullDuplex(boolean fullDuplex) {
return this;
}

@Override
public <T> Builder putHttpExecutionAttribute(SdkHttpExecutionAttribute<T> attribute, T value) {
this.executionAttributesBuilder.put(attribute, value);
return this;
}

@Override
public Builder httpExecutionAttributes(SdkHttpExecutionAttributes executionAttributes) {
Validate.paramNotNull(executionAttributes, "executionAttributes");
this.executionAttributesBuilder = executionAttributes.toBuilder();
return this;
}


@Override
public AsyncExecuteRequest build() {
return new AsyncExecuteRequest(this);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

package software.amazon.awssdk.http;


import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

import nl.jqno.equalsverifier.EqualsVerifier;
import org.junit.jupiter.api.Test;

class SdkHttpExecutionAttributesTest {

@Test
void equalsAndHashcode() {
EqualsVerifier.forClass(SdkHttpExecutionAttributes.class)
.withNonnullFields("attributes")
.verify();
}

@Test
void getAttribute_shouldReturnCorrectValue() {
SdkHttpExecutionAttributes attributes = SdkHttpExecutionAttributes.builder()
.put(TestExecutionAttribute.TEST_KEY_FOO, "test")
.build();
assertThat(attributes.attribute(TestExecutionAttribute.TEST_KEY_FOO)).isEqualTo("test");
}

private static final class TestExecutionAttribute<T> extends SdkHttpExecutionAttribute<T> {

private static final TestExecutionAttribute<String> TEST_KEY_FOO = new TestExecutionAttribute<>(String.class);

private TestExecutionAttribute(Class valueType) {
super(valueType);
}
}
}
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@
<rxjava.version>2.2.21</rxjava.version>
<commons-codec.verion>1.10</commons-codec.verion>
<jmh.version>1.29</jmh.version>
<awscrt.version>0.15.8</awscrt.version>
<awscrt.version>0.15.15</awscrt.version>

<!--Test dependencies -->
<junit5.version>5.8.1</junit5.version>
Expand Down
5 changes: 0 additions & 5 deletions services-custom/s3-transfer-manager/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,6 @@
<artifactId>aws-core</artifactId>
<version>${awsjavasdk.version}</version>
</dependency>
<dependency>
<groupId>software.amazon.awssdk</groupId>
<artifactId>metrics-spi</artifactId>
<version>${awsjavasdk.version}</version>
</dependency>
<dependency>
<groupId>software.amazon.awssdk</groupId>
<artifactId>service-test-utils</artifactId>
Expand Down
Loading