Skip to content

Release v0.15.15#435

Merged
TingDaoK merged 8 commits intoawslabs:mainfrom
aceeric:release-v0.15.15
Feb 28, 2022
Merged

Release v0.15.15#435
TingDaoK merged 8 commits intoawslabs:mainfrom
aceeric:release-v0.15.15

Conversation

@aceeric
Copy link
Copy Markdown
Contributor

@aceeric aceeric commented Jan 30, 2022

This PR supports the ability to perform S3 get and put operations over HTTP as well as HTTPS, and with custom ports. This PR integrates with changes to the aws-c-s3 repo/submodule made in this commit: awslabs/aws-c-s3@2a37467

@TingDaoK
Copy link
Copy Markdown
Contributor

TingDaoK commented Feb 8, 2022

Do you also update the submodule aws-c-s3? Can you open a PR to that as well?

@aceeric
Copy link
Copy Markdown
Contributor Author

aceeric commented Feb 9, 2022

@TingDaoK Hello - per your request please see PR: awslabs/aws-c-s3#174

@aceeric
Copy link
Copy Markdown
Contributor Author

aceeric commented Feb 13, 2022

@TingDaoK Hello - I updated this PR and the corresponding one here: awslabs/aws-c-s3#174 to pass the URI into the C code.

Comment thread src/native/s3_client.c Outdated
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0.
*/
#include "aws/io/uri.h"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you use the same pattern to include the dependency? So, something like #include <aws/io/uri.h>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok done

Comment thread src/native/s3_client.c Outdated
AWS_ZERO_STRUCT(endpoint);
if (jni_endpoint != NULL) {
struct aws_byte_cursor endpoint_str = aws_jni_byte_cursor_from_jbyteArray_acquire(env, jni_endpoint);
AWS_FATAL_ASSERT(aws_uri_init_parse(&endpoint, allocator, &endpoint_str) == AWS_OP_SUCCESS);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, I think we can handle this error. That should not crash the program.

Suggested change
AWS_FATAL_ASSERT(aws_uri_init_parse(&endpoint, allocator, &endpoint_str) == AWS_OP_SUCCESS);
if (aws_uri_init_parse(&endpoint, allocator, &endpoint_str)) {
aws_jni_throw_runtime_exception(env, "S3Client.aws_s3_client_make_meta_request: failed to parse endpoint");
aws_jni_byte_cursor_from_jbyteArray_release(env, jni_endpoint, endpoint_str);
goto done;
}

Also, for proper clean up, you need to move the endpoint part after request_message created, as we clean up the resources from done tag

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done - handled the error very slightly differently to call aws_jni_byte_cursor_from_jbyteArray_release only once

Copy link
Copy Markdown
Contributor

@TingDaoK TingDaoK left a comment

Choose a reason for hiding this comment

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

looks good! Thank you!

@aceeric
Copy link
Copy Markdown
Contributor Author

aceeric commented Feb 23, 2022

I noticed the Windows build was failing. It was complaining about ref to meta_request. I build on Linux no issues. Even so I moved the declaration to the top of the function so I don't see a way the Windows compiler can complain about it now

@aceeric
Copy link
Copy Markdown
Contributor Author

aceeric commented Feb 27, 2022

@TingDaoK Hello - Thank you for your help - is there anything else needed from me to get this into the main branch?

@TingDaoK TingDaoK merged commit f5a7955 into awslabs:main Feb 28, 2022
@TingDaoK
Copy link
Copy Markdown
Contributor

Nope, thank you. I merged for you, let me know if you need a new release to use it from maven.

@aceeric
Copy link
Copy Markdown
Contributor Author

aceeric commented Feb 28, 2022

Super - thank you. If you can release to Maven we would appreciate it because we have some in-house code that presently uses a patched JAR and it would be better to use the official Maven release version. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants