Skip to content

Conversation

@seanmcc-msft
Copy link
Member

@seanmcc-msft seanmcc-msft commented Jan 10, 2020

Resolves #9186

@seanmcc-msft seanmcc-msft marked this pull request as ready for review January 10, 2020 22:25
/// The approximate size of the data stored in bytes, rounded up to the nearest gigabyte. Note that this value may not include all recently created or recently resized files.
/// </summary>
public int ShareUsageBytes { get; internal set; }
public long ShareUsageInBytes { get; internal set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

One example of this situation from BCL is Array.Length vs Array.LongLength, I'm not sure that we should use the same naming pattern but just FYI.

/// <summary>
/// Warning: Share usage may exceed int.MaxValue. Use ShareUsageInBytes instead.
/// </summary>
public int ShareUsageBytes {
Copy link
Member

Choose a reason for hiding this comment

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

Nit - use [EditorBrowsable(Never)] here so folks don't even see this in intellisense.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

if (ShareUsageInBytes > int.MaxValue)
{
#pragma warning disable CA1065 // Do not raise exceptions in unexpected locations
throw new OverflowException("ShareUsageInBytes exceeds int.MaxValue. Use ShareUsageInBytes instead.");
Copy link
Member

Choose a reason for hiding this comment

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

Change to "ShareUsageBytes exceeds int.MaxValue. Use ShareUsageInBytes instead." and move to Constants

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

/// <summary>
/// Creates a new PermissionInfo instance for mocking.
/// </summary>
public static ShareStatistics ShareStatistics(int shareUsageBytes)
Copy link
Member

Choose a reason for hiding this comment

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

Nit - use [EditorBrowsable(Never)] here so folks don't even see this in intellisense.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

{
Transport = new MockTransport(mockResponse)
};
ShareClient shareClient = new ShareClient(new Uri(TestConfigDefault.FileServiceEndpoint), shareClientOption);
Copy link
Member

Choose a reason for hiding this comment

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

This still needs to be instrumented for the sync/async to work correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

@seanmcc-msft
Copy link
Member Author

/azp run net - storage -ci

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@seanmcc-msft seanmcc-msft merged commit c28a98c into Azure:master Jan 13, 2020
@seanmcc-msft seanmcc-msft deleted the feature/storage/GetStatsBug branch January 13, 2020 21:55
@kemmis
Copy link

kemmis commented Jan 13, 2020

Nice job team. I’m very grateful to have experienced this process. #PleaseHireMeMicrosoft 🤣

👏🙏

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.

[BUG] ShareClient.GetStatistics(); can't handle more bytes than can be stored in an Int.

5 participants