-
Notifications
You must be signed in to change notification settings - Fork 909
Add support for AwsResponseMetadata #701
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
Conversation
What does the customer experience look like, accessing the data? I'd expect |
Here's the test: https://github.com/aws/aws-sdk-java-v2/pull/701/files#diff-c598a7406fc83ad8b0cc5ff552173b69 Basically, private void verifyResponseMetadata(S3Response response) {
S3ResponseMetadata s3ResponseMetadata = response.responseMetadata();
assertThat(s3ResponseMetadata).isNotNull();
assertThat(s3ResponseMetadata.metadata().size()).isEqualTo(3);
assertThat(s3ResponseMetadata.requestId()).isNotEqualTo("UNKNOWN");
assertThat(s3ResponseMetadata.extendedRequestId()).isNotEqualTo("UNKNOWN");
} yeah, |
|
Unfortunately, we cannot use
I thought it would be useful for customers who want to retrieve all metadata without doing multiple method calls. As to the naming, how about |
Removed the logic of subclassing of responsehandler to extract metadata since simpleDb is the only services has additional metadata and we no longer support simpledb in v2. All headers are being put to the
|
* data. | ||
*/ | ||
protected void registerAdditionalMetadataExpressions(StaxUnmarshallerContext unmarshallerContext) { | ||
if (!metadata.containsKey(AWS_REQUEST_ID)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed if we put all headers into the map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is still needed for ec2 because the requestId for it is extracted from xml.
import software.amazon.awssdk.services.kinesis.model.DescribeLimitsResponse; | ||
import software.amazon.awssdk.services.kinesis.model.KinesisResponse; | ||
|
||
public class KinesisResponseMetadataIntegrationTest extends AbstractTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinesis actually has an id-2 like S3. Can we customize for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
* Contains information needed to create a {@link StaxResponseHandler}. | ||
*/ | ||
@SdkProtectedApi | ||
public final class StaxOperationMetadata { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move the unmarshaller into this param like class? Or do you wanna keep it separate to avoid including type params?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it like this to follow the same pattern as json protocol.
Lines 68 to 71 in 2fa64ca
public <T> JsonResponseHandler<T> createResponseHandler( | |
JsonOperationMetadata operationMetadata, | |
Unmarshaller<T, JsonUnmarshallerContext> responseUnmarshaller) { | |
return getSdkFactory().createResponseHandler(operationMetadata, responseUnmarshaller); |
But we can move the unmarshaller into this class. Don't have strong opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like the current pattern as it keeps the metadata object a simple POJO with no type params.
4529f5f
to
ad47a11
Compare
ad47a11
to
7ec5558
Compare
…8308313e Pull request: release <- staging/fd170742-9fef-4b30-810c-ee148308313e
Description
Add
AwsResponseMetadata
support to allow customers to retrieve metadata information such asrequestId
,hostId
for debugging purpose.see #670
streaming operation looks like the following:
non-streaming operation:
Pending
I've include the
DefaultS3Client
,S3ResponseMetadata
,S3ResponseHanlder
in the PR.ProposedCodegen Changes:customResponseMetadata
is a map of metadata key to header key.S3ResponseMetadata
andwill be generated based on it.S3ResponseHandler
Currently it only supports header, let me know if there are other places we can retrieve additional metadata.
Types of changes
Checklist
mvn install
succeedsLicense