search

keboola / db-extractor-mssql

MIT License
1 stars 2 forks source link

add option to disable fallback #117

Closed pivnicek closed 5 years ago

pivnicek commented 5 years ago

Fixes #116

One thing that I don't like about this is that BCP is not retried.

MiroCillik commented 5 years ago

Why it is not retried?

pivnicek commented 5 years ago

Thanks @MiroCillik . ugh

Adding a simple test that the PDO fallback didn't happen shouldn't be too hard.

There are 2 new tests there that do exactly this, did you miss them?

pivnicek commented 5 years ago

lol, sorry miro! I'd forgotten to commit them 🤦‍♂ ! Pushed now, thanks

pivnicek commented 5 years ago

Adding this option has nothing to do with retries or?

Well, there's no retry wrapper on the BCP, it retries the fallback if that fails. Not sure if it should maybe be for another issue/PR though. Curious to @tomasfejfar thoughts.

tomasfejfar commented 5 years ago

I think we really need to cleanup the "main loop" first - extract and isolate the BCP and PDO into their own classes or at least methods. BCP has it's own class, but only for the extraction itself. There is ton of other code in the try/catch block still.

I may try to do it, if you want...

image

MiroCillik commented 5 years ago

@pivnicek lol, I just looked at it and thought that I must've been blind yesterday :D OK it's fine with the tests. Tomas is right about cleaning "the main loop" a little bit.

pivnicek commented 5 years ago

Yeah @tomasfejfar I agree about the cleanup (give it a go by all means! :) ). But I think it should be in a different PR.

tomasfejfar commented 5 years ago

Certainly in different PR.