Make hive temporary staging directory to be configurable #12199

kokosing · 2019-01-09T06:48:35Z

No description provided.

findepi · 2019-01-09T08:10:12Z

Looks good, but Travis is red.

Make hive temporary staging directory location to be configurable commit could include some explanation eg In some HDFS deployments, /tmp cannot be used to write temporary files

kokosing · 2019-01-09T08:52:35Z

@findepi Thanks. Updated.

presto-hive/src/main/java/com/facebook/presto/hive/HiveWriteUtils.java

wenleix · 2019-01-09T19:44:43Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveLocationService.java

    {
-        // skip using temporary directory for S3
-        return !isS3FileSystem(context, hdfsEnvironment, path);
+        return isTemporaryStagingDirectoryEnabled(session)


Maybe want to update the comment around WriteMode enum?

presto/presto-hive/src/main/java/com/facebook/presto/hive/LocationHandle.java

Lines 107 to 121 in 79f480b

public enum WriteMode

{

/**

* common mode for new table or existing table (both new and existing partition)

*/

STAGE_AND_MOVE_TO_TARGET_DIRECTORY(false),

/**

* for new table in S3

*/

DIRECT_TO_TARGET_NEW_DIRECTORY(true),

/**

* for existing table in S3 (both new and existing partition)

*/

DIRECT_TO_TARGET_EXISTING_DIRECTORY(true),

/**/;

nezihyigitbasi

Make hive temporary staging directory to be disableable

I feel like we can update the commit title to:Add config flag to disable hive temp staging directory or Make use of hive temp staging directory configurable.

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

nezihyigitbasi · 2019-01-09T20:35:26Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

@@ -1190,4 +1192,17 @@ public HiveClientConfig setS3SelectPushdownMaxConnections(int s3SelectPushdownMa
        this.s3SelectPushdownMaxConnections = s3SelectPushdownMaxConnections;
        return this;
    }
+
+    @Config("hive.temporary-staging-directory-enabled")


How about we rename this to hive.use-temporary-staging-directory-for-writes?

could we be using staging directory for reads? would there be use for this?

@findepi : Unlikely. When the WriteMode is STAGE_AND_MOVE_TO_TARGET_DIRECTORY, the data is first written to the stage directory, and rename to the target directory when when commit the write.

But I don't feel we have to emphasize for-write? What about hive.use-temporary-staging-directory ?

But I don't feel we have to emphasize for-write?

i thought the same

What about hive.use-temporary-staging-directory ?

would do
however, i think the original hive.temporary-staging-directory-enabled is slightly better. Consider the case when one uses s3 where staging cannot (currently) be used.
When users sets hive.use-temporary-staging-directory = true, they could expect that staging is used (or error is reported). However, none of these will happen.
At the same time, I don't want to report an error when the option is used with s3 -- a single cluster may be talking to s3 and hdfs and user may want to set the option to change behavior on hdfs.

So... to me this should be either

hive.temporary-staging-directory-enabled (original) or

hive.hdfs.use-temporary-staging-directory

i prefer the original.

Using hdfs could make someone to think that this only affects actual HDFS, and not any HDFS-compatible file system.

I would keep the original. @nezihyigitbasi What do you think?

I feel like "using" a staging directory sounds better than "enabling" it (hive.use-temporary-staging-directory). But, if you all want to go with the original name I am fine with it.

nezihyigitbasi · 2019-01-09T20:38:09Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSessionProperties.java

@@ -76,6 +76,7 @@
    private static final String COLLECT_COLUMN_STATISTICS_ON_WRITE = "collect_column_statistics_on_write";
    private static final String OPTIMIZE_MISMATCHED_BUCKET_COUNT = "optimize_mismatched_bucket_count";
    private static final String S3_SELECT_PUSHDOWN_ENABLED = "s3_select_pushdown_enabled";
+    private static final String TEMPORARY_STAGING_DIRECTORY_ENABLED = "temporary_staging_directory_enabled";


Similarly, should we rename this to use_temporary_staging_directory_for_writes?

nezihyigitbasi

Make hive temporary staging directory location to be configurable

Make hive temporary staging directory location configurable

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveSessionProperties.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

kokosing

@nezihyigitbasi Comments addressed, would you mind to take a look?

kokosing · 2019-01-14T14:14:25Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

@@ -1190,4 +1192,17 @@ public HiveClientConfig setS3SelectPushdownMaxConnections(int s3SelectPushdownMa
        this.s3SelectPushdownMaxConnections = s3SelectPushdownMaxConnections;
        return this;
    }
+
+    @Config("hive.temporary-staging-directory-enabled")


Using hdfs could make someone to think that this only affects actual HDFS, and not any HDFS-compatible file system.

I would keep the original. @nezihyigitbasi What do you think?

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

nezihyigitbasi

LGTM. (Please merge after the release is completed.)

wenleix

LGTM.

wenleix · 2019-01-14T22:00:58Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

+        assertEquals(hiveInsertTableHandle.getLocationHandle().getWritePath(), hiveInsertTableHandle.getLocationHandle().getTargetPath());
+
+        session = Session.builder(getSession())
+                .setCatalogSessionProperty("hive", "temporary_staging_directory_path", "/tmp/custom/temporary-${USER}")


nit: Maybe explicitly put "temporary_staging_directory_enabled": "true"?

I am asking this because I got confused when looking at it first time, then I realized temporary_staging_directory_enabled is by default true.

Presto for some file systems that wrap S3, should behave in the same way as it does for S3.

In some deployments, /tmp cannot be used to write temporary files.

Fix formatting

16b3ea9

facebook-github-bot added the CLA Signed label Jan 9, 2019

kokosing requested a review from electrum January 9, 2019 08:52

wenleix reviewed Jan 9, 2019

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/HiveWriteUtils.java Outdated Show resolved Hide resolved

wenleix reviewed Jan 9, 2019

View reviewed changes

nezihyigitbasi reviewed Jan 9, 2019

View reviewed changes

kokosing commented Jan 14, 2019

View reviewed changes

nezihyigitbasi approved these changes Jan 14, 2019

View reviewed changes

wenleix approved these changes Jan 14, 2019

View reviewed changes

kokosing added 2 commits January 15, 2019 13:47

Make use of hive temp staging directory configurable

f75bf56

Presto for some file systems that wrap S3, should behave in the same way as it does for S3.

Make hive temporary staging directory location configurable

88d2c61

In some deployments, /tmp cannot be used to write temporary files.

kokosing merged commit 0052bd7 into prestodb:master Jan 25, 2019

kokosing mentioned this pull request Jan 25, 2019

Make hive temporary staging directory location configurable trinodb/trino#70

Merged

kokosing deleted the origin/master/052_s3_tmp branch January 29, 2019 10:27

wenleix mentioned this pull request Jan 31, 2019

Fix creating empty non-bucketed Hive partition #12204

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make hive temporary staging directory to be configurable #12199

Make hive temporary staging directory to be configurable #12199

kokosing commented Jan 9, 2019

findepi commented Jan 9, 2019

kokosing commented Jan 9, 2019

wenleix Jan 9, 2019

nezihyigitbasi left a comment

nezihyigitbasi Jan 9, 2019

findepi Jan 9, 2019

wenleix Jan 13, 2019 •

edited

Loading

findepi Jan 13, 2019

kokosing Jan 14, 2019

nezihyigitbasi Jan 14, 2019

nezihyigitbasi Jan 9, 2019

nezihyigitbasi left a comment

kokosing left a comment

kokosing Jan 14, 2019

nezihyigitbasi left a comment

wenleix left a comment

wenleix Jan 14, 2019

	public enum WriteMode
	{
	/**
	* common mode for new table or existing table (both new and existing partition)
	*/
	STAGE_AND_MOVE_TO_TARGET_DIRECTORY(false),
	/**
	* for new table in S3
	*/
	DIRECT_TO_TARGET_NEW_DIRECTORY(true),
	/**
	* for existing table in S3 (both new and existing partition)
	*/
	DIRECT_TO_TARGET_EXISTING_DIRECTORY(true),
	/**/;

Make hive temporary staging directory to be configurable #12199

Make hive temporary staging directory to be configurable #12199

Conversation

kokosing commented Jan 9, 2019

findepi commented Jan 9, 2019

kokosing commented Jan 9, 2019

Choose a reason for hiding this comment

nezihyigitbasi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenleix Jan 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nezihyigitbasi left a comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nezihyigitbasi left a comment

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenleix Jan 13, 2019 •

edited

Loading