Skip to content

Split out ForceCloseHandles into two methods#8134

Merged
JoshLove-msft merged 5 commits into
Azure:masterfrom
JoshLove-msft:close-handles
Oct 18, 2019
Merged

Split out ForceCloseHandles into two methods#8134
JoshLove-msft merged 5 commits into
Azure:masterfrom
JoshLove-msft:close-handles

Conversation

@JoshLove-msft
Copy link
Copy Markdown
Member

Fixes #8075

We should probably look into how to add test cases for when handles are actually open.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit - can we swap the parameter ordering for marker and recursive? We should leave marker just in case for completeness, but it's pretty useless now that we're looping for the user.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This needs to change. We're going to wrap this in a loop that continues executing until no additional marker is returned or it throws.

@JoshLove-msft JoshLove-msft force-pushed the close-handles branch 2 times, most recently from 2eef790 to 69bf4d7 Compare October 16, 2019 20:50
Copy link
Copy Markdown
Member

@tg-msft tg-msft left a comment

Choose a reason for hiding this comment

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

A few style nits, but it looks good

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit - extra line

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I actually added that intentionally to separate the return a bit as it was kind of hard to read with the while condition right above it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit - add the parameters one per line

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure

@JoshLove-msft
Copy link
Copy Markdown
Member Author

/azp run net - storage - ci

1 similar comment
@JoshLove-msft
Copy link
Copy Markdown
Member Author

/azp run net - storage - ci

@JoshLove-msft
Copy link
Copy Markdown
Member Author

/azp run net - storage - ci

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@JoshLove-msft JoshLove-msft merged commit 8a44c77 into Azure:master Oct 18, 2019
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.

[P1] Storage: Split ForceCloseHandles into two methods

2 participants