Add hive session property to enable writing staging files #12386

shixuan-fan · 2019-02-26T02:08:58Z

This is part of the effort on recoverable grouped execution (#12124).

wenleix · 2019-02-26T05:08:27Z

Maybe we shouldn't reference issue in commit. If you look at prestodb/presto#12124 , there are many commits (during code review) refer this issue :)

shixuan-fan · 2019-02-26T17:48:53Z

I though cross-linking would be helpful for tracking purpose but didn't realize the commits are also shown in the issue. This seems an undesired feature from github :( I'm not sure if removing references now would be helpful. I think we should probably link to the issue when PR is ready to merge. But anyway there should be a kill-switch for github #featurerequest

wenleix · 2019-02-26T18:42:57Z

@shixuan-fan : The problem is every time you rebase, it is considered as a different commit :). And dropping it now still helps :)

shixuan-fan · 2019-02-26T18:47:28Z

I don't think dropping it helps sadly because my latest push was still reflected on the issue.

shixuan-fan · 2019-02-26T19:57:29Z

Addressed offline. It is about commit message, and I misunderstood as reference in pull request 😂

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

wenleix

"Extract partitionUpdates deserialization logic"

Looks good.

wenleix

"Support staging file commit"

generally looks good. per offline discussion, we don't want to name it staging file commit-- staging file is very specific to Hive. The real "purpose" is that this connector supports retryable lifespan updates -- and use staging file is the "way". I will think about it :)

wenleix · 2019-02-27T02:09:58Z

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java

+    @Override
+    public boolean isWritingStagingFilesEnabled(Session session)
+    {
+        CatalogMetadata catalogMetadata = getCatalogMetadataForWrite(session, session.getCatalog().orElseThrow(() -> new IllegalStateException("No catalog specified")));


nit: I prefer to have line break to help read

CatalogMetadata catalogMetadata = getCatalogMetadataForWrite( session, session.getCatalog() .orElseThrow(() -> new IllegalStateException("No catalog specified")));

What if catalog was specified explicitly?

Say

INSERT INTO prism.table SELECT FROM ...

I would expect this method to accept catalogName

presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java

wenleix

"Add hive session property to enable writing staging files"

Two quick comments. I haven't looked into HiveStagingFileCommitter and tests.

My general impression is SemiTransactionalHiveMetastore doesn't need to face StagingFileInfo, right?

...to-hive/src/main/java/com/facebook/presto/hive/metastore/SemiTransactionalHiveMetastore.java

wenleix · 2019-02-27T04:25:19Z

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

-                missingFileNamesBuilder.add(fileName);
+            String targetFileName = HiveWriterFactory.computeBucketedFileName(filePrefix, i) + fileExtension;
+            if (!targetFileNames.contains(targetFileName)) {
+                missingFileNamesBuilder.add(new StagingFileInfo(targetFileName, targetFileName));


What's the purpose of this (instead of returning a List<String> ? :) -- Looks like they get called into createEmptyFile anyway, which can take List<String>

The problem here is we need to merge empty partition's PartitionUpdate with non-empty partitions's PartitionUpdate for later processing. See computePartitionUpdatesForMissingBuckets and the caller in finishCreateTable and finishInsert.

Of course we could leave createEmptyFile and computeFileNamesForMissingBuckets untouched.

arhimondr

Extract partitionUpdates deserialization logic

LGTM

shixuan-fan · 2019-02-28T19:34:25Z

@wenleix Yes I think you are right. Let me see if I could remove this dependency.

arhimondr

Some comments

arhimondr · 2019-02-28T19:17:02Z

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java

+    @Override
+    public boolean isWritingStagingFilesEnabled(Session session)
+    {
+        CatalogMetadata catalogMetadata = getCatalogMetadataForWrite(session, session.getCatalog().orElseThrow(() -> new IllegalStateException("No catalog specified")));


What if catalog was specified explicitly?

Say

INSERT INTO prism.table SELECT FROM ...

I would expect this method to accept catalogName

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java

presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java

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

...to-hive/src/main/java/com/facebook/presto/hive/metastore/SemiTransactionalHiveMetastore.java

wenleix · 2019-03-01T04:56:57Z

cc @elonazoulay for some context on renaming stage files.

shixuan-fan · 2019-03-02T01:02:42Z

Thanks for reviewing @arhimondr and @wenleix. I addressed most of the comments and left some comments in unresolved states so people could revisit.

wenleix

"Move part of asyncRename logic into MetastoreUtil"

Looks good except minor comment.

...to-hive/src/main/java/com/facebook/presto/hive/metastore/SemiTransactionalHiveMetastore.java

wenleix

"Add hive session property to enable writing staging files"

Looks good % minor comments.

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

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

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

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

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

presto-hive/src/main/java/com/facebook/presto/hive/metastore/MetastoreUtil.java

arhimondr · 2019-03-08T20:59:34Z

...to-hive/src/main/java/com/facebook/presto/hive/metastore/SemiTransactionalHiveMetastore.java

-                MoreFutures.getFutureValue(fileRenameFuture, PrestoException.class);
+            ListenableFuture<?> fileRenameFutureAggregate = allAsList(fileRenameFutures);
+            try {
+                MoreFutures.getFutureValue(fileRenameFutureAggregate);


nit: static import?

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

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

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

Moved logic: - getFileSystem - renameFile - waitForAsyncRename

shixuan-fan · 2019-03-08T22:16:05Z

Comments addressed.

arhimondr

Some questions / suggestions. No action needed.

arhimondr · 2019-03-09T16:42:38Z

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

@@ -127,6 +126,7 @@ public HiveMetadataFactory(
        this.locationService = requireNonNull(locationService, "locationService is null");
        this.tableParameterCodec = requireNonNull(tableParameterCodec, "tableParameterCodec is null");
        this.partitionUpdateCodec = requireNonNull(partitionUpdateCodec, "partitionUpdateCodec is null");
+        this.fileRenameExecutor = listeningDecorator(requireNonNull(executorService, "executorService is null"));


Why don't we just bind this executor as ListeningExecutorService?

arhimondr · 2019-03-09T16:50:48Z

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

+    @Provides
+    public ExecutorService createFileRanemeExecutor(HiveConnectorId hiveClientId, HiveClientConfig hiveClientConfig)
+    {
+        return newFixedThreadPool(


The disadvantage of the fixedThreadPool is that it keeps the entire thread pool alive all the time. That increases the memory footprint.

I think the listeningDecorator(new ExecutorServiceAdapter(new BoundedExector(newCachedThreadPool(daemonThreadsNamed("hive-" + hiveClientId + "-%s")), threadLimit))) would be a better option.

arhimondr · 2019-03-09T17:03:38Z

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

+            }
+        }
+
+        waitForListenableFutures(commitFutures);


Just some thoughts.

I can see how the waitForListenableFutures saves the code lines, and follows the DRY principles. However the semantics of this method are very unclear. It is very hard to know what does this method do. Without looking into the method guts it is impossible to give answers to these questions:

Single future has failed? Does it fail? Does it fail fast?

Does it cancel the features in case of a failure?

How are the failures propagated?

I don't have a strong opinion here. But it seems to me that in such tricky cases sometimes it is better to leave a few lines more, but use the standard library methods (that engineers are more likely to know semantics of).

But again, no action is required here. Those are just thoughts.

I agree with @arhimondr . Especially given it's only used twice, inline it seems to be reasonable.

While DRY principal is in general a good practice, there are cases it doesn't apply. It really depends on:

Does the refactor/abstraction natural ? -- does it make things difficult to follow ?

How evil is to repeat your-self in this case?

Are these repeating real semantic repeat, or it's just the code look very similar?

Now I think about it, I understand there is no Guava method that does this waitForListenableFutures -- I was very confused before since it looks like a very common use case :). Thanks @arhimondr for noting this is a "simple method in disguise".

wenleix · 2019-03-10T06:54:08Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/MetastoreUtil.java

+    {
+        ListenableFuture<?> listenableFutureAggregate = whenAllSucceed(listenableFutures).call(() -> null, directExecutor());
+        try {
+            getFutureValue(listenableFutureAggregate);


I think you want to do getFutureValue(listenableFutureAggregate, PrestoException.class)

shixuan-fan · 2019-03-11T17:03:40Z

I'll create a separate PR to address these remaining comments :). Thanks for raising them up! @arhimondr @wenleix

shixuan-fan requested review from wenleix and arhimondr February 26, 2019 02:08

facebook-github-bot added the CLA Signed label Feb 26, 2019

shixuan-fan assigned wenleix Feb 26, 2019

wenleix assigned arhimondr and wenleix and unassigned wenleix and arhimondr Feb 26, 2019

shixuan-fan force-pushed the file_commit branch from b60f7f2 to fa7fcb9 Compare February 26, 2019 18:45

shixuan-fan force-pushed the file_commit branch from fa7fcb9 to a879b69 Compare February 26, 2019 19:57

shixuan-fan force-pushed the file_commit branch 2 times, most recently from 192612d to 11a4085 Compare February 26, 2019 23:54

wenleix reviewed Feb 27, 2019

View reviewed changes

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

wenleix reviewed Feb 27, 2019

View reviewed changes

shixuan-fan force-pushed the file_commit branch from 11a4085 to 917a680 Compare February 27, 2019 17:53

arhimondr reviewed Feb 28, 2019

View reviewed changes

shixuan-fan force-pushed the file_commit branch from 917a680 to 776a71c Compare March 2, 2019 00:19

shixuan-fan force-pushed the file_commit branch 2 times, most recently from 320e286 to 78466f0 Compare March 4, 2019 18:19

wenleix reviewed Mar 6, 2019

View reviewed changes

...to-hive/src/main/java/com/facebook/presto/hive/metastore/SemiTransactionalHiveMetastore.java Outdated Show resolved Hide resolved

wenleix approved these changes Mar 6, 2019

View reviewed changes

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

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

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

wenleix assigned arhimondr and unassigned wenleix Mar 6, 2019

wenleix reviewed Mar 6, 2019

View reviewed changes

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

wenleix reviewed Mar 6, 2019

View reviewed changes

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

wenleix reviewed Mar 6, 2019

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/metastore/MetastoreUtil.java Show resolved Hide resolved

wenleix assigned shixuan-fan Mar 6, 2019

shixuan-fan force-pushed the file_commit branch 3 times, most recently from 229ffed to be09eef Compare March 6, 2019 22:01

shixuan-fan added 2 commits March 6, 2019 16:06

Extract partitionUpdates deserialization logic

1894c1c

Extract rename error messsage generation logic

adf904d

shixuan-fan force-pushed the file_commit branch 5 times, most recently from aea5c54 to dc37747 Compare March 8, 2019 00:55

arhimondr reviewed Mar 8, 2019

View reviewed changes

shixuan-fan added 3 commits March 8, 2019 14:14

Use ListenableFuture in SemiTransactionalHiveMetastore

04cf676

Move part of asyncRename logic into MetastoreUtil

488c05b

Moved logic: - getFileSystem - renameFile - waitForAsyncRename

Add hive session property to enable writing staging files

6309dc3

shixuan-fan force-pushed the file_commit branch from dc37747 to 6309dc3 Compare March 8, 2019 22:14

shixuan-fan merged commit e629ad1 into prestodb:master Mar 9, 2019

shixuan-fan deleted the file_commit branch March 9, 2019 00:00

arhimondr reviewed Mar 9, 2019

View reviewed changes

wenleix reviewed Mar 10, 2019

View reviewed changes

shixuan-fan mentioned this pull request Mar 11, 2019

Minor refactors in HiveClientModule and MetastoreUtil #12463

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hive session property to enable writing staging files #12386

Add hive session property to enable writing staging files #12386

shixuan-fan commented Feb 26, 2019 •

edited

Loading

wenleix commented Feb 26, 2019 •

edited

Loading

shixuan-fan commented Feb 26, 2019 •

edited

Loading

wenleix commented Feb 26, 2019 •

edited

Loading

shixuan-fan commented Feb 26, 2019

shixuan-fan commented Feb 26, 2019

wenleix left a comment

wenleix left a comment •

edited

Loading

wenleix Feb 27, 2019

arhimondr Feb 28, 2019

wenleix left a comment

wenleix Feb 27, 2019

shixuan-fan Mar 4, 2019 •

edited

Loading

arhimondr left a comment

shixuan-fan commented Feb 28, 2019

arhimondr left a comment

arhimondr Feb 28, 2019

wenleix commented Mar 1, 2019

shixuan-fan commented Mar 2, 2019

wenleix left a comment

wenleix left a comment

arhimondr Mar 8, 2019

shixuan-fan commented Mar 8, 2019

arhimondr left a comment

arhimondr Mar 9, 2019

arhimondr Mar 9, 2019

arhimondr Mar 9, 2019

wenleix Mar 10, 2019 •

edited

Loading

wenleix Mar 10, 2019

shixuan-fan commented Mar 11, 2019

Add hive session property to enable writing staging files #12386

Add hive session property to enable writing staging files #12386

Conversation

shixuan-fan commented Feb 26, 2019 • edited Loading

wenleix commented Feb 26, 2019 • edited Loading

shixuan-fan commented Feb 26, 2019 • edited Loading

wenleix commented Feb 26, 2019 • edited Loading

shixuan-fan commented Feb 26, 2019

shixuan-fan commented Feb 26, 2019

wenleix left a comment

Choose a reason for hiding this comment

wenleix left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this 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

shixuan-fan Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

arhimondr left a comment

Choose a reason for hiding this comment

shixuan-fan commented Feb 28, 2019

arhimondr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenleix commented Mar 1, 2019

shixuan-fan commented Mar 2, 2019

wenleix 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

shixuan-fan commented Mar 8, 2019

arhimondr left a 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

wenleix Mar 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shixuan-fan commented Mar 11, 2019

shixuan-fan commented Feb 26, 2019 •

edited

Loading

wenleix commented Feb 26, 2019 •

edited

Loading

shixuan-fan commented Feb 26, 2019 •

edited

Loading

wenleix commented Feb 26, 2019 •

edited

Loading

wenleix left a comment •

edited

Loading

shixuan-fan Mar 4, 2019 •

edited

Loading

wenleix Mar 10, 2019 •

edited

Loading