Skip to content

Fix lack of valid class loader for TableChangesSplitProcessor in Delta/Iceberg#19849

Merged
findepi merged 3 commits intotrinodb:masterfrom
homar:homar/fix_table_changes
Nov 22, 2023
Merged

Fix lack of valid class loader for TableChangesSplitProcessor in Delta/Iceberg#19849
findepi merged 3 commits intotrinodb:masterfrom
homar:homar/fix_table_changes

Conversation

@homar
Copy link
Copy Markdown
Member

@homar homar commented Nov 21, 2023 •
edited
Loading

Description

Adding missing ClassLoaderSafeTableFunctionProcessorProvider and ClassLoaderSafeTableFunctionSplitProcessor
Also refactoring ConnectorTableFunction binding in Hive.

Additional context and related issues

Lack of usage of ClassLoaderSafe.. may have resulted in queries failure when some criterias were met:

  • data was stored on hdfs
  • query involved table function that used either TableFunctionProcessorProvider or TableFunctionSplitProcessor or both
  • query was the first query touching hdfs that was run on the node after it started
    In such case trino would fail to instantiate specific FileSystem and query would fail

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Section
* Fix issue when table function could fail if its invocation was a first query run a on worker ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Nov 21, 2023
@github-actions github-actions bot added tests:hive iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector labels Nov 21, 2023
@homar homar changed the title Homar/fix table changes Add missing ClassLoaderSafe and refactor ConnectorTableFunction binding in Hive Nov 21, 2023
@homar homar force-pushed the homar/fix_table_changes branch from c883583 to 2c463c2 Compare November 21, 2023 14:08
@homar
Copy link
Copy Markdown
Member Author

homar commented Nov 21, 2023

Failure is not related #16315

@homar homar marked this pull request as ready for review November 21, 2023 15:37
findepi
findepi reviewed Nov 21, 2023
View reviewed changes
plugin/trino-hive/src/main/java/io/trino/plugin/hive/InternalHiveConnectorFactory.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Nov 21, 2023

Choose a reason for hiding this comment

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

Change HiveConnector.functionProvider not to be Optional

core/trino-main/src/main/java/io/trino/operator/table/Sequence.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Nov 21, 2023

Choose a reason for hiding this comment

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

is't not needed here, is it?

...c/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeTableFunctionSplitProcessor.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Nov 21, 2023

Choose a reason for hiding this comment

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

update TestClassLoaderSafeWrappers

...ain/java/io/trino/plugin/deltalake/functions/tablechanges/TableChangesProcessorProvider.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Nov 21, 2023

Choose a reason for hiding this comment

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

add line break before getClass().getClassLoader()

...va/io/trino/plugin/iceberg/functions/tablechanges/TableChangesFunctionProcessorProvider.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Nov 21, 2023

Choose a reason for hiding this comment

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

add line break before getClass().getClassLoader()

...ain/java/io/trino/plugin/deltalake/functions/tablechanges/TableChangesProcessorProvider.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Nov 21, 2023

Choose a reason for hiding this comment

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

is it fixing a bug?
would you be able to describe the bug being fixed in end-user visible terms?

Copy link
Copy Markdown
Member Author

@homar homar Nov 21, 2023

Choose a reason for hiding this comment

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

Added to description

Copy link
Copy Markdown
Member

@findepi findepi Nov 21, 2023

Choose a reason for hiding this comment

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

thanks!
please update the Release notes section too

Copy link
Copy Markdown
Member

@findepi findepi Nov 22, 2023

Choose a reason for hiding this comment

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

discussed offline; this is a bug fix

please change the commit title from "Introduce ClassLoaderSafeTableFunctionSplitProcessor" to something that indicates it is a fix (perhaps "Fix ....")

core/trino-main/src/main/java/io/trino/operator/table/ExcludeColumns.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Nov 21, 2023

Choose a reason for hiding this comment

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

What is this for here?

core/trino-main/src/main/java/io/trino/operator/table/Sequence.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Nov 21, 2023

Choose a reason for hiding this comment

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

is't not needed here, is it?

...ain/java/io/trino/plugin/base/classloader/ClassLoaderSafeTableFunctionProcessorProvider.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Nov 21, 2023

Choose a reason for hiding this comment

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

final

Copy link
Copy Markdown
Member Author

@homar homar Nov 21, 2023

Choose a reason for hiding this comment

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

Others are not final, why to make an exception here?

Copy link
Copy Markdown
Member

@findepi findepi Nov 21, 2023

Choose a reason for hiding this comment

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

because it should be final -- or is the class supposed to be subclassed?

...ain/java/io/trino/plugin/base/classloader/ClassLoaderSafeTableFunctionProcessorProvider.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Nov 21, 2023

Choose a reason for hiding this comment

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

update TestClassLoaderSafeWrappers

@homar homar force-pushed the homar/fix_table_changes branch from 2c463c2 to 20afb5c Compare November 21, 2023 17:34
@homar
Copy link
Copy Markdown
Member Author

homar commented Nov 21, 2023

Again failure is not related #16315

@homar homar force-pushed the homar/fix_table_changes branch 2 times, most recently from 2298946 to f7d249f Compare November 21, 2023 20:26
@homar homar force-pushed the homar/fix_table_changes branch from f7d249f to 48446c8 Compare November 22, 2023 08:50
@findepi findepi merged commit 19327e9 into trinodb:master Nov 22, 2023
@github-actions github-actions bot added this to the 434 milestone Nov 22, 2023
@findepi findepi changed the title Add missing ClassLoaderSafe and refactor ConnectorTableFunction binding in Hive Fix lack of valid class loader for TableChangesSplitProcessor in Delta/Iceberg Nov 22, 2023
@findepi findepi mentioned this pull request Nov 22, 2023
Merged
@mosabua mosabua mentioned this pull request Nov 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@findepi findepi findepi approved these changes

@kokosing kokosing Awaiting requested review from kokosing

@lukasz-walkiewicz lukasz-walkiewicz Awaiting requested review from lukasz-walkiewicz

Assignees

No one assigned

Labels

cla-signed delta-lake Delta Lake connector hive Hive connector iceberg Iceberg connector

Milestone

434

Development

Successfully merging this pull request may close these issues.

2 participants

@homar @findepi