Skip to content

Ensure that batchParamValues is cleared in all cases when executing a batch#1869

Merged
Jeffery-Wasty merged 4 commits intomicrosoft:mainfrom
ianroberts:clear-batch-on-fail-v2
Aug 30, 2022
Merged

Ensure that batchParamValues is cleared in all cases when executing a batch#1869
Jeffery-Wasty merged 4 commits intomicrosoft:mainfrom
ianroberts:clear-batch-on-fail-v2

Conversation

@ianroberts
Copy link
Copy Markdown
Contributor

This is an alternative implementation to #1768 which moves the batchParamValues = null to a new top-level try/finally block.

Closes #1767

@ianroberts
Copy link
Copy Markdown
Contributor Author

It looks like a lot of changes but it's mostly whitespace as I've had to indent the whole method bodies by an extra 4 spaces to wrap them in a try block.

@Jeffery-Wasty
Copy link
Copy Markdown
Contributor

Hi @ianroberts,

Just a comment. We haven't forgotten about this, but for the time being priority is being given to preparing for our upcoming release. Expect to see this addressed shortly thereafter.

@Jeffery-Wasty Jeffery-Wasty self-assigned this Aug 11, 2022
@Jeffery-Wasty Jeffery-Wasty added the Under Review Used for pull requests under review label Aug 11, 2022
…atch, whether via bulk update or via traditional batch insert.

Closes microsoft#1767

Signed-off-by: Ian Roberts <[email protected]>
@ianroberts ianroberts force-pushed the clear-batch-on-fail-v2 branch from 7206950 to 094c584 Compare August 17, 2022 09:26
@Jeffery-Wasty
Copy link
Copy Markdown
Contributor

I've cleaned up the tests slightly, but otherwise this perfectly reproduces, resolves, and confirms resolution of, the issue. Thank you so much!

@Jeffery-Wasty Jeffery-Wasty added this to the 11.3.0 milestone Aug 17, 2022
Jeffery-Wasty
Jeffery-Wasty previously approved these changes Aug 17, 2022
@Jeffery-Wasty Jeffery-Wasty self-requested a review August 17, 2022 19:30
@Jeffery-Wasty
Copy link
Copy Markdown
Contributor

Almost ready, can we ask that you run the files through your ide, formatting using the format files included at the root. We think there may be some minor spacing issues.

@ianroberts
Copy link
Copy Markdown
Contributor Author

I don't have Eclipse (I'm an IntelliJ user) but I was able to apply the mssql-jdbc_formatter.xml format using https://code.revelc.net/formatter-maven-plugin/ - it changed 117 files in total but most of them were just cases where it stripped trailing spaces off lines (mostly blank lines in the middle of Javadoc comments). It made one indentation change to the test file I touched in this PR so I've committed that change now.

@ianroberts ianroberts force-pushed the clear-batch-on-fail-v2 branch from 9c2b8a7 to 9ce74b3 Compare August 18, 2022 18:50
@Jeffery-Wasty
Copy link
Copy Markdown
Contributor

Thank you so much. With that I can ask the rest of the team to review, and we can finally get this merged.

@Jeffery-Wasty Jeffery-Wasty self-requested a review August 23, 2022 21:12
@tkyc
Copy link
Copy Markdown
Contributor

tkyc commented Aug 24, 2022

We'll need to run the PR through some of our internal pipelines.

@Jeffery-Wasty Jeffery-Wasty merged commit 07248a8 into microsoft:main Aug 30, 2022
@Jeffery-Wasty Jeffery-Wasty removed the Under Review Used for pull requests under review label Aug 30, 2022
@Jeffery-Wasty Jeffery-Wasty removed their assignment Sep 1, 2022
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.

Batch not cleared on failure when using bulk update

4 participants