Add Oracle connection pool

[eskabetxe]

as asked here: #1959 (comment)

this is to allow to configure a oracle connection pool

[Praveen2112]

Can we have an interface for ConnectionProvider and have implementation for for both pooledDataSource and a driver based one and bind them in the run time ?

[eskabetxe]

hi @Praveen2112, I change this factory to be only a pool factory..
for driver based I leave the DriverConnectorFactory that is already used..
this was a old code from previous PR.

if you could check this changes.

[tooptoop4]

@eskabetxe can u change

private final int fetchSize = 1000;
to 10000 or at least 5000?

[eskabetxe]

@tooptoop4 I create a PR with your request

[losipiuk]

A few cleanup comments mostly looks good.

[losipiuk]

Two more nits. Thanks.
@Praveen2112, @ebyhr are you good with this one already?

[electrum]

Can you squash the last three commits?

[eskabetxe]

@losipiuk could this be merged?

[Praveen2112]

Sorry for the delayed response. Looks good to me . But can we move the integration test changes as a separate commit

[eskabetxe]

@Praveen2112 done

[losipiuk]

Checkstyle failed.
I think what @Praveen2112 asked for was to separate integration tests refactoring (extracting base class) from adding tests for connection pooling. It would be a bit nicer for review. Though given fact review is done already I will not push for that.

[eskabetxe]

@losipiuk fixed
@Praveen2112 I generate a commit with the extraction of base class..

[findepi]

This is being added in Extract a variable with Oracle JDBC version commit. Does not look intentional.

(Not sure why would you want to separate this btw, but maybe this was agreed upon)

[eskabetxe]

was asked here #3770 (comment)
I let the new dependency because extracting only the version will lead to a "why extract the version if only used in one dependency"

[losipiuk]

I think that at the time i suggested extracting the commit there was just one use of dep.oracle.version. But maybe I missed the other one.

Anyway.

• Either leave commit which extract variable and only use it for ojdbc8 entry. And then add ucp in followup commit together with pooling implementation.
• Or squash two commits together.

I am slightly toward former one.

[findepi]

the Oracle IntegrationTest cleanups should not be a separate commit
(we should not add TestOraclePoolIntegrationSmokeTest being a copy of TestOracleIntegrationSmokeTest` and only then extract common super class)

[eskabetxe]

should I squash the 3 commits?

[losipiuk]

It should be either:

2 commits:

• rafactor tests extractin BaseOracleIntegrationSmokeTest (still with 1 concrete subclass TestOracleIntegrationSmokeTest)
• implement pooling and add TestOraclePoolIntegrationSmokeTest

Or just 1 commit which refactors tests, add pooling implementation and TestOraclePoolIntegrationSmokeTest alltogether.

[Praveen2112]

I guess the OracleQueryRunner changes can be treated as a separate commit while other the tests can be a part of Add Oracle connection pool

[findepi]

(just skimming)

[losipiuk]

@eskabetxe there are two minor comments still.

• one about variable extraction in pom.xml: Add Oracle connection pool #3770 (comment)
• the other one about splitting tests refactoring around commits: Add Oracle connection pool #3770 (comment)

Are you planning to address those? It would be nice to merge this one eventually :)

[eskabetxe]

@losipiuk sorry for the delay..
its done...

[losipiuk]

Thanks. Looks good modulo checkstyle violation:

2020-07-06T19:18:59.5653900Z [INFO] There is 1 error reported by Checkstyle 8.32 with checkstyle/airbase-checks.xml ruleset.
2020-07-06T19:18:59.5678894Z [ERROR] src/test/java/io/prestosql/plugin/oracle/OracleQueryRunner.java:[16,8] (imports) UnusedImports: Unused import - com.google.common.collect.ImmutableList.

[eskabetxe]

@losipiuk fixed

[losipiuk]

Merged, thanks!