Skip to content

Conversation

@mattBrzezinski
Copy link
Member

Overview

This merge request is for the port of AWSCredentials as part of the re-write. I'm currently happy with the state that this component is in so it is almost a 1 to 1 mapping. There are a couple of changes however;

  1. Removal of LazyJSON in favour of JSON. I don't see a point in using multiple JSON packages, so I removed LazyJSON.
  2. Since we are still dependent on AWSCore at this stage we run into naming conflicts for AWSCredentials. You can see the qualifiers here and through the test/AWSCredentials.jl package, I have left reminders to remove the qualification afterwards.
  3. Since we are using AWSCore.aws_config() to create the configuration object, it uses AWSCore.AWSCredentials object, there are cases which this breaks tests such as here. I have marked these tests as @test_broken, and left a comment above. The alternative would be to take these credential objects, and re-created them under the AWS module. But since the next merge request will be for creating AWSConfig, I figured it's quicker just to do it this way.

Next Steps

The next merge request will be to create the AWSConfig object, being based off of this proposal by Alex.

@omus
Copy link
Member

omus commented Apr 9, 2020

Removal of LazyJSON in favour of JSON. I don't see a point in using multiple JSON packages, so I removed LazyJSON.

I believe LazyJSON was used for performance reasons. It would be good to do some benchmarking.

@mattBrzezinski
Copy link
Member Author

mattBrzezinski commented Apr 17, 2020

Removal of LazyJSON in favour of JSON. I don't see a point in using multiple JSON packages, so I removed LazyJSON.

I believe LazyJSON was used for performance reasons. It would be good to do some benchmarking.

Below is a benchmark comparison between JSON.jl and LazyJSON.jl when loading the deps/metadata.json file.

Time Comparison:

Package Average Median Run 1 Run 2 Run 3 Run 4 Run 5
LazyJSON.jl 0.2714742 0.270885 0.282056 0.274130 0.270885 0.263565 0.266735
JSON.jl 0.8045014 0.802492 0.799345 0.804900 0.814626 0.802492 0.801144

LazyJSON.jl is 0.5330272 seconds faster on average compared to JSON.jl Memory is constant between all runs with LazyJSON.jl using 72.440 MiB and JSON.jl using 132.162 MiB.

I'll create a new commit moving from JSON.jl to LazyJSON.jl across this repo. The only downside to LazyJSON.jl is that it is not well maintained, but we can take it under our wing while we're at it.

using JSON
using LazyJSON

path = joinpath(@__DIR__, "metadata.json")

function _lazyjson()
    file = String(read(path))
    LazyJSON.parse(file)
end

function _json()
    JSON.parsefile(path)
end


@time _lazyjson()
@time _json()

@mattBrzezinski
Copy link
Member Author

I spent the last little while trying to convert from using JSON to LazyJSON, and at this time I don't think it's worth putting in the effort.

LazyJSON does not support a lot of functionality which JSON does, such as setindex!(), get(), etc. and it is slowly evolving into re-writing a lot of the code which is already in v1. I can see now why AWSCore.jl is using a combination of these packages.

end

info = get(info, "Errors", info)
info = get(info, "Error", info)
Copy link
Member

Choose a reason for hiding this comment

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

This code is a little bizarre. Right now all of these would unpack in the same way:

info = Dict("Errors" => 1, "Error", => 2)
info = Dict("Errors" => Dict("Error", => 2))
info = Dict("Error", => 2)

What is the structure you want to unpack?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just ported over from AWSCore.jl, to me it looks like AWS is returning either one or multiple errors in their response and this is just handling both of those cases.

I'll play around with this more but I believe that AWS will only return back Errors or Error but not both. I'll write some more tests to capture all these different cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated this to have an XML and JSON request testing, I've spent some time looking around but I can't seem to find any examples online of other request errors that AWS throws back and their structures.

I'm a bit wary of updating this since it is working currently in AWSCore.jl and no one has logged an issue about it. Seems like this knowledge has been lost a bit. :(

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I guess leave it as is for now then

Copy link
Contributor

@nicoleepp nicoleepp left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

I'm good to proceed with this

@mattBrzezinski mattBrzezinski merged commit 7f0c5fb into v1 Apr 27, 2020
@mattBrzezinski mattBrzezinski deleted the MB/aws-credentials branch April 27, 2020 18:49
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.

4 participants