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

Install JDBC drivers for Delta product tests at runtime and upgrade docker-images to version 81 #17667

Merged
merged 3 commits into from
Jul 27, 2023

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented May 29, 2023

Description

This upgrades Delta Lake version to 2.4.0.
https://github.com/delta-io/delta/releases/tag/v2.4.0

Release notes

(x) This is not user-visible or docs only and no release notes are required.

@ebyhr ebyhr added the no-release-notes This pull request does not require release notes entry label May 29, 2023
@cla-bot cla-bot bot added the cla-signed label May 29, 2023
@ebyhr ebyhr marked this pull request as draft May 29, 2023 01:28
@ebyhr ebyhr force-pushed the ebi/docker-images-81 branch 7 times, most recently from 5e155d3 to 1663c8a Compare June 8, 2023 02:44
@ebyhr ebyhr marked this pull request as ready for review June 8, 2023 02:48
@ebyhr ebyhr changed the title Upgrade docker-images to version 81 Install JDBC drivers for Delta product tests at runtime and upgrade docker-images to version 81 Jun 8, 2023
@ebyhr
Copy link
Member Author

ebyhr commented Jun 8, 2023

CI hit #17782 & #16416

@ebyhr ebyhr requested review from electrum and findepi June 8, 2023 05:14
@ebyhr ebyhr force-pushed the ebi/docker-images-81 branch 3 times, most recently from d804d9a to 1ea96c3 Compare June 19, 2023 06:44
@ebyhr ebyhr force-pushed the ebi/docker-images-81 branch 2 times, most recently from cb71c89 to c4abb2c Compare June 20, 2023 00:52
@ebyhr ebyhr force-pushed the ebi/docker-images-81 branch from c4abb2c to bd3522c Compare June 20, 2023 07:22
@ebyhr
Copy link
Member Author

ebyhr commented Jun 21, 2023

Addressed comments.

@ebyhr ebyhr force-pushed the ebi/docker-images-81 branch from bd3522c to 09dc558 Compare July 3, 2023 06:28
@ebyhr
Copy link
Member Author

ebyhr commented Jul 5, 2023

testDatabricksVacuumRemoveChangeDataFeedFiles didn't pass because DeltaQueryExecutor opens a new connection every time.

@findinpath findinpath self-requested a review July 5, 2023 16:01
@ebyhr ebyhr force-pushed the ebi/docker-images-81 branch from 09dc558 to 7fd5966 Compare July 10, 2023 04:04
@ebyhr ebyhr self-assigned this Jul 11, 2023
@ebyhr ebyhr force-pushed the ebi/docker-images-81 branch 2 times, most recently from 6b5492d to 1183780 Compare July 11, 2023 21:33
@ebyhr
Copy link
Member Author

ebyhr commented Jul 13, 2023

CI hit #18266

@ebyhr
Copy link
Member Author

ebyhr commented Jul 14, 2023

CI hit #14239

@ebyhr ebyhr requested a review from findepi July 14, 2023 09:08
@ebyhr
Copy link
Member Author

ebyhr commented Jul 19, 2023

@findepi Could you take another look?

@findepi
Copy link
Member

findepi commented Jul 20, 2023

I added some code sample how we can avoid regressions and shared state.

However, why not simply configure io.trino.tempto.query.JdbcConnectivityParamsState#jar?
Looks like Tempto has the support for what we need already?

@ebyhr
Copy link
Member Author

ebyhr commented Jul 20, 2023

I already tried JdbcConnectivityParamsState#jar, but it didn't work. I'm losing the root cause though.

@findepi
Copy link
Member

findepi commented Jul 21, 2023

I already tried JdbcConnectivityParamsState#jar, but it didn't work.

let's understand why. It looks like the "correct" approach to do what we're trying to do.
Of course requires a jar to be provided at runtime. Was getting the correct path to the jar a problem?

@ebyhr
Copy link
Member Author

ebyhr commented Jul 21, 2023

The path was correct. It looks classloader issue. Changing io.trino.tempto.internal.query.JdbcUtils#getDriverClassLoader to like below will resolve it:

    private static ClassLoader getDriverClassLoader(JdbcConnectivityParamsState jdbcParamsState)
    {
        try {
            if (!jdbcParamsState.jar.isPresent()) {
                return JdbcUtils.class.getClassLoader();
            }
            URL jarURL = new File(jdbcParamsState.jar.get()).toURL();
            return URLClassLoader.newInstance(new URL[] {jarURL}, ClassLoader.getPlatformClassLoader());
        }
        catch (MalformedURLException e) {
            throw new RuntimeException("could not create classloader for file" + jdbcParamsState.jar.get(), e);
        }
    }

@ebyhr ebyhr force-pushed the ebi/docker-images-81 branch from bd0fd16 to 0536cad Compare July 26, 2023 22:13
ebyhr added 2 commits July 27, 2023 14:22
This upgrades Delta Lake version to 2.4.0

Change delta.minWriterVersion to 7. Otherwise, Spark throws
delta.minWriterVersion needs to be one of 1, 2, 3, 4, 5, 7 error.
@ebyhr ebyhr force-pushed the ebi/docker-images-81 branch from 0536cad to bec8b7d Compare July 27, 2023 05:27
@ebyhr ebyhr merged commit 3979bbf into master Jul 27, 2023
@ebyhr ebyhr deleted the ebi/docker-images-81 branch July 27, 2023 09:22
@github-actions github-actions bot added this to the 423 milestone Jul 27, 2023
Comment on lines +49 to +52
@GuardedBy("DRIVERS")
private static final Map<String, String> DRIVERS = new HashMap<>();
@GuardedBy("JDBC_EXECUTORS")
private static final Map<JdbcConnectivityParamsState, JdbcQueryExecutor> JDBC_EXECUTORS = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

if we use tempto driver jar feature, why is the state maintained explicitly?

please use tempto-configuration.yaml to configure drivers

Copy link
Member Author

Choose a reason for hiding this comment

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

Sent #18464

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

None yet

2 participants