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

use a ParameterBinder to pass snapshot into the merge statement and get rid of the stored procedure. #21

Merged
merged 3 commits into from
May 12, 2015

Conversation

mwkohout
Copy link

@mwkohout mwkohout commented May 7, 2015

Some DBAs frown on stored procedures. This change eliminates the need for the stored procedure.

And locally some tests were failing for me because timeouts were too short. I increased some of the timeouts in those tests.

I haven't updated the library's documents, so let me know if you want that as part of the change

@bpg
Copy link

bpg commented May 10, 2015

Oracle's MERGE won't work for BLOBs greater than some internal buffer size. See #9 for more details.

@mwkohout
Copy link
Author

Have you executed the tests in your environment? In mine they all pass including 'Macbeth'.

If not, what version of the oracle drivers are you using?

@bpg
Copy link

bpg commented May 10, 2015

It was 100% reproducible in the project's test environment on v 1.0.9. You can checkout bfa643b and run test-oracle.sh.

@mwkohout
Copy link
Author

I don't disagree that passing the clob as a string was incorrect, but this patch works differently.

Have you checked out my alterations and run them? Also, if you have and they failed, could you share what version of Oracle you ran them against?

@mwkohout
Copy link
Author

I noticed that these tests weren't actually asserting the value of the saved snapshots when they were retrieved.

@dnvriend
Copy link
Contributor

Thanks for your contribution and the discussion. Is there any problem with the code, the way it is, and/or is there any risk changing it back to the MERGE INTO statement? If not, I will merge the pull request.

@bpg
Copy link

bpg commented May 11, 2015

@mwkohout, the code from your branch seems to be working in all the tests. I think the ParameterBinder did the trick.
Thanks for fixing that, I don't really like the SP approach as well :)

@mwkohout
Copy link
Author

I don't think there are any new risks. I think those same risks would exist when using the stored procedure version of this code..

@dnvriend
Copy link
Contributor

Thank you both. That settles it I will merge it asap.

@dnvriend dnvriend merged commit 5cf3fff into akka:master May 12, 2015
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.

3 participants