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

[#7383] Added parameter CommandTimeOut to the function Copy-DbaDbTableData #7384

Conversation

mrlmachado
Copy link
Contributor

@mrlmachado mrlmachado commented May 27, 2021

Type of Change

  • Bug fix (non-breaking change, fixes # )
  • New feature (non-breaking change, adds functionality, fixes [Feature] Add CommandTimeout parameter to Copy-DbaDbTableData #7383
  • Breaking change (effects multiple commands or functionality, fixes # )
  • Ran manual Pester test and has passed (`.\tests\manual.pester.ps1)
  • Adding code coverage to existing functionality
  • Pester test is included
  • If new file reference added for test, has is been added to github.com/sqlcollaborative/appveyor-lab ?
  • Nunit test is included
  • Documentation
  • Build system

Purpose

Sometimes the reader command runs indefinitely and needs to have a timeout. This change adds this as a parameter to the function

Approach

Adding a parameter to a function

@andreasjordan
Copy link
Contributor

You will have to add the parameter to the test as well.

Changing documentation
@mrlmachado
Copy link
Contributor Author

thanks @andreasjordan , I fixed it

@@ -4773,6 +4773,15 @@
"5000",
""
],
[
Copy link
Collaborator

Choose a reason for hiding this comment

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

Welcome and thanks for the contribution!! Can you remove this file from the PR please - this one gets auto generated by the module.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, indeed thank you. Also, cool avatar!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you guys and sorry for the delay. Working on it now

$knownParameters += [System.Management.Automation.PSCmdlet]::CommonParameters

(@(Compare-Object -ReferenceObject ($knownParameters | Where-Object {$_}) -DifferenceObject $params).Count ) | Should -Be 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also could you remove the whitespace please - there is a Invoke-DbatoolsFormatter that might help - or some .vscode settings in the code also (if you're using vscode)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whitespaces were removed by vscode when I saved the file. Do you want me to revert them?

@@ -94,6 +94,9 @@ function Copy-DbaDbTableData {
.PARAMETER BulkCopyTimeOut
Value in seconds for the BulkCopy operations timeout. The default is 5000 seconds.

.PARAMETER CommandTimeOut
Value in seconds for the reader command operations timeout. The default is 0 seconds (no timeout).
Copy link
Member

Choose a reason for hiding this comment

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

This has an effect on more than the reader command since we pass the connection string with the command timeout set to the BulkCopy object. Bulk copy operation will still adhere to the command timeout, if the network packets hit the command timeout then that bulk copy will fail as well.

Suggested change
Value in seconds for the reader command operations timeout. The default is 0 seconds (no timeout).
Gets or sets the wait time (in seconds) before terminating the attempt to execute the reader and bulk copy operation. The default is 0 seconds (no timeout).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, I liked it. Will change now

Improving description of commandtimeout parameter
@andreasjordan
Copy link
Contributor

Can we change "TimeOut" to "Timeout"?
https://docs.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlbulkcopy.bulkcopytimeout
says its "BulkCopyTimeout", so change that as well.

@mrlmachado
Copy link
Contributor Author

Can we change "TimeOut" to "Timeout"?
https://docs.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlbulkcopy.bulkcopytimeout
says its "BulkCopyTimeout", so change that as well.

Nice suggestion. I actually saw that before creating it, but I just kept that standard from the BulkCopyTimeOut. I should have changed it but didn't want to be as forward as this. Will change now

@andreasjordan
Copy link
Contributor

One last change: "$bulkCopy.BulkCopyTimeOut" to "$bulkCopy.BulkCopyTimeout" please.

@mrlmachado
Copy link
Contributor Author

One last change: "$bulkCopy.BulkCopyTimeOut" to "$bulkCopy.BulkCopyTimeout" please.

Done! 😄

Copy link
Contributor

@andreasjordan andreasjordan 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 to me.

@mrlmachado
Copy link
Contributor Author

Thanks! How new releases works? When can I expect to have a new version with this new functionality? Sorry to ask this, just to have an idea really.

@wsmelton wsmelton merged commit 50996a2 into dataplat:development Jun 6, 2021
@mrlmachado mrlmachado deleted the feature/copydbadbtabledata_commandtimeout branch June 6, 2021 02:53
@potatoqualitee
Copy link
Member

@mrlmachado - it's in the gallery now 🥳 We're likely going to be releasing every other day or so unless there's a batch of PRs then I'll just do it daily

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.

[Feature] Add CommandTimeout parameter to Copy-DbaDbTableData
6 participants