Skip to content

[HUDI-6550] Add Hadoop conf to HiveConf for HiveSyncConfig #9221

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.

Sign up for GitHub

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

Merged
yihua merged 1 commit into from
Sep 14, 2023
Merged

[HUDI-6550] Add Hadoop conf to HiveConf for HiveSyncConfig #9221

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,9 @@ public HiveSyncConfig(Properties props) {

public HiveSyncConfig(Properties props, Configuration hadoopConf) {
super(props, hadoopConf);
HiveConf hiveConf = hadoopConf instanceof HiveConf
? (HiveConf) hadoopConf : new HiveConf(hadoopConf, HiveConf.class);
HiveConf hiveConf = new HiveConf();
// HiveConf needs to load Hadoop conf to allow instantiation via AWSGlueClientFactory
hiveConf.addResource(hadoopConf);
Copy link
Member

@xushiyan xushiyan Jul 25, 2023

Choose a reason for hiding this comment

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

not so sure if this is equivalent to holding the original hadoopConf as this changes the order of addResources() during constructing. We should be good only if we can verify the equivalence.

Copy link
Member

@xushiyan xushiyan Jul 25, 2023

Choose a reason for hiding this comment

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

i think the ideal approach is to make the passed-in hiveConf load hadoop conf properly to use AWSGlueClientFactory at the very beginning (when creating hive sync config) so that nothing needs to load at this point. cc @yihua

Copy link
Contributor Author

@CTTY CTTY Jul 28, 2023

Choose a reason for hiding this comment

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

Loading AWSGlueClientFactory property specifically should solve the issue on AWS side, but it's possible that there are other configs/custom configs passed in via Spark session, and won't be loaded by hard-coded logic. I still think loading entire hadoop conf here would be a safer choice.

And this change could change the order of adding resource for hive conf. I've gone thru the HiveConf constructor but didn't see any usage of resource during constructing so I think it shouldn't affect, maybe I've overlooked something?
An alternative solution would be always pass hadoopConf to HiveConf constructor. Wdyt?

Suggested change
hiveConf.addResource(hadoopConf);
HiveConf hiveConf = new HiveConf(hadoopConf, HiveConf.class);

Copy link
Contributor

@danny0405 danny0405 Aug 6, 2023

Choose a reason for hiding this comment

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

but it's possible that there are other configs/custom configs passed in via Spark session,

Is this a classical way people pass around hive options with spark?

An alternative solution would be always pass hadoopConf to HiveConf constructor

Does it introduce too much overhead then?

Copy link
Contributor Author

@CTTY CTTY Aug 7, 2023

Choose a reason for hiding this comment

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

Hi @danny0405,

  1. It's not the preferred way, but it might be the best way in cases like serverless applications since those can't modify hive-site.xml easily
  2. There is overhead to load hadoop conf to hive conf instead of casting it directly, but it's almost negligible imo

Copy link
Member

@xushiyan xushiyan Aug 21, 2023

Choose a reason for hiding this comment

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

@CTTY always creating a new HiveConf looks good to me.

setHadoopConf(hiveConf);
validateParameters();
}
Expand Down