Skip to content
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

Async Keys implementation #6457

Merged
merged 8 commits into from
Jun 6, 2019
Merged

Async Keys implementation #6457

merged 8 commits into from
Jun 6, 2019

Conversation

maririos
Copy link
Member

@maririos maririos commented Jun 1, 2019

Async implementation of:

  • Create
  • Get
  • Update
  • Delete
  • Purge
  • Restore
  • Backup
  • Import

Similar to what we did for Secrets API

@maririos maririos added KeyVault Client This issue points to a problem in the data-plane of the library. labels Jun 1, 2019
@maririos maririos self-assigned this Jun 1, 2019
@maririos maririos changed the title Keys implementation for create, update, get Async Keys implementation Jun 3, 2019
return true;
}

private static bool AreEqual(byte[] a, byte[] b)
Copy link
Member

@KrzysztofCwalina KrzysztofCwalina Jun 3, 2019

Choose a reason for hiding this comment

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

Instead of writing this routine by hand, use Span.SequenceEqual. It's much more efficient.

{
internal void Deserialize(Stream content)
{
using (JsonDocument json = JsonDocument.Parse(content, default))
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, any particular reason to not use the JsonSerializer?

@schaabs
Copy link
Member

schaabs commented Jun 4, 2019

public enum KeyType : uint

A Key cannot have more than one type. This shouldn't be a flags enumeration


Refers to: sdk/keyvault/Azure.Security.KeyVault.Keys/src/KeyType.cs:13 in 6788f44. [](commit_id = 6788f446d1aa1b60bf2e4621f9129167b00a68c2, deletion_comment = False)

@schaabs
Copy link
Member

schaabs commented Jun 4, 2019

    Other = 0x0020,

Given that all the enums in this assembly need to support values which are not explicitly in the enumeration perhaps they should be structs instead of simple enums. This way we could also store the actual string in the case we can't parse them.


Refers to: sdk/keyvault/Azure.Security.KeyVault.Keys/src/KeyType.cs:20 in 6788f44. [](commit_id = 6788f446d1aa1b60bf2e4621f9129167b00a68c2, deletion_comment = False)

{
var writer = new ArrayBufferWriter<byte>();

var json = new Utf8JsonWriter(writer);
Copy link

Choose a reason for hiding this comment

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

If serializing things is done frequently, you might consider pooling writers (if you are using a version of the package where they're a class) via ConcurrentBag (or some other pooling technique). It reduces the GC impact of creating the write buffers.

Of course, then you need a strategy for knowing when to not return an object to the pool... but maybe "always pool" is fine.

@maririos
Copy link
Member Author

maririos commented Jun 4, 2019

PR Updated with feedback

@maririos
Copy link
Member Author

maririos commented Jun 5, 2019

Is there something else I should address? or are we good to go to merge this as the first iteration of the Keys async implementation

@maririos
Copy link
Member Author

maririos commented Jun 6, 2019

FYI @schaabs issue for enum: #6506

@maririos maririos merged commit f8c8b84 into Azure:master Jun 6, 2019
mentat9 pushed a commit to mentat9/azure-sdk-for-net that referenced this pull request Jun 10, 2019
@maririos maririos deleted the implementation branch July 11, 2019 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants