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

Jump to bottom

Rename files written by PageSink #14974

Merged

highker merged 6 commits into prestodb:master from NikhilCollooru:m2

Oct 1, 2020

Contributor

NikhilCollooru commented Aug 6, 2020 •

edited

Loading

This PR enables the workers to send / receive metadata requests /responses from coordinator.
If this feature is enabled for Hive, then all the files written by the HivePageSink for a given partition will have increasing whole numbers as file names. (For ex: Lets say this we enabled this feature and ds=2020-09-30 is written by HivePageSink. Then we can expect the file names of that partition to be 0,1,2,3..N etc ).

depended by https://github.com/facebookexternal/presto-facebook/pull/1125

== RELEASE NOTES ==
Hive Changes
* Add support for file renaming for Hive connector. This can be enabled with ``hive.file_renaming_enabled`` configuration property

NikhilCollooru requested a review from highker

August 6, 2020 02:19

Contributor

highker commented Aug 6, 2020

rebase a bit maybe?

NikhilCollooru force-pushed the m2 branch 2 times, most recently from 5b2a34e to 3e915b5 Compare

August 7, 2020 01:25

highker reviewed

View reviewed changes

Contributor

highker left a comment

High-level design direction:

Create a new interface in ConnectorMetadata called updateMetadata in SPI
Create a new service HiveFileRenamer for Hive connector in presto-hive and inject it into HiveMetadata. This is to move your current logic in ContinuousTaskStatusFetcher into this HiveFileRenamer. So that you can call the interface updateMetadata to tunnel the logic into it.
Change the field name in TaskStatus to be called MetadataUpdates.
Create a new connector interface ConnectorDistributedMetadataUpdater in SPI. This is to replace FileNameProvider. Make sure to name the interface in a generic way (to avoid word like 'file', 'name', etc). This class is not injected. But to created from class loader. (Check ConnectorPlanOptimizerProvider as an example).
Create a new class HiveDistributedMetadataUpdater to inherit ConnectorPlanOptimizerProvider. This is to replace FileNameManager. Move all your logic there.
SqlTask should call HiveDistributedMetadataUpdater to update the renamed files.
HiveDistributedMetadataUpdater will be tunneled into HivePageSink just like FileNameProvider.

NikhilCollooru force-pushed the m2 branch 6 times, most recently from 0dbf2bf to c89651f Compare

August 16, 2020 17:10

NikhilCollooru requested a review from highker

August 16, 2020 19:24

highker marked this pull request as ready for review

August 17, 2020 16:36

highker reviewed

View reviewed changes

Contributor

highker left a comment

comment on SPI change. Once we fix SPI, other parts should be easy to proceed

presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorPageSinkProvider.java Outdated Show resolved Hide resolved

presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadata.java Outdated Show resolved Hide resolved

presto-spi/src/main/java/com/facebook/presto/spi/MetadataUpdateRequest.java Outdated Show resolved Hide resolved

...spi/src/main/java/com/facebook/presto/spi/connector/ConnectorDistributedMetadataUpdater.java Outdated Show resolved Hide resolved

presto-spi/src/main/java/com/facebook/presto/spi/MetadataUpdateResult.java Outdated Show resolved Hide resolved

NikhilCollooru force-pushed the m2 branch from c89651f to b9687fb Compare

August 18, 2020 04:48

NikhilCollooru requested a review from highker

August 18, 2020 06:32

highker reviewed

View reviewed changes

Contributor

highker left a comment

still reviewing

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

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

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

presto-hive/src/main/java/com/facebook/presto/hive/HiveDistributedMetadataUpdater.java Outdated

Comment on lines 42 to 43

    
                  @Inject

                  public HiveDistributedMetadataUpdater() {}

Contributor

highker Aug 18, 2020 •

edited

Loading

We might need to inject something light-weighted to track the lifecycle of a query. SqlTaskManager is the closest I can think of. But that is too heavy and not accessible through connectors. We may in the end hook callbacks from HivePageSink to remove dead queries.
clean up hashmap by hooking it to HiveFileWriter::commit/rollback
add an assertion to any integration or smoke tests to assert we don't leak requests. Check 9f89256 as an example

highker reviewed

View reviewed changes

Contributor

highker left a comment

take a look ConnectorPlanOptimizer and ConnectorPlanOptimizerProvider
ConnectorDistributedMetadataUpdater <-> ConnectorPlanOptimizer. It will be per query/task
Create a new ConnectorDistributedMetadataUpdaterProvider that is a singleton. Inject it into LocalExecutionPlanner and somehow return it back SqlTask?.
In LocalExecutionPlanner, when you create a new TableWriterOperatorFactory, create a new ConnectorDistributedMetadataUpdater instance and pass it in.

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

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

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

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

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

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

highker reviewed

View reviewed changes

Contributor

highker left a comment

Also, if we pass the updater into table writer instead of injecting it, we will need to modify the SPI createPageSink. To do it, we could rename PageSinkProperties into PageSinkContext as a separate commit. Then put the updater inside PageSinkContext so we don't have to change the interface again.

NikhilCollooru force-pushed the m2 branch 4 times, most recently from 00f121b to 5100427 Compare

August 25, 2020 16:44

NikhilCollooru requested a review from highker

August 25, 2020 18:20

highker reviewed

View reviewed changes

Contributor

highker left a comment

"Rename PageSinkProperties to PageSinkContext" LGTM

highker reviewed

View reviewed changes

Contributor

highker left a comment

"Add ConnectorMetadataUpdater to handle worker's metadata updates"

there is a typo":wq" in the commit message
haven't reviewed in details yet. One quick comment: we probably need to use a session property to guard if we actually wanna do send/receive any requests for HiveMetadataUpdater.

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

...to-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadataUpdaterProvider.java Outdated Show resolved Hide resolved

...to-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadataUpdaterProvider.java Outdated Show resolved Hide resolved

presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadataUpdater.java Outdated Show resolved Hide resolved

presto-main/src/main/java/com/facebook/presto/execution/TaskMetadataContext.java Outdated Show resolved Hide resolved

presto-main/src/main/java/com/facebook/presto/execution/TaskMetadataContext.java Outdated Show resolved Hide resolved

highker reviewed

View reviewed changes

Contributor

highker left a comment

"Add ConnectorMetadataUpdateHandle resolver" LGTM

highker reviewed

View reviewed changes

Contributor

highker left a comment

High-level comments of this PR: we need tests; a lot of unit tests and integration tests. Probably enable the feature for all write tests for hive connector. We also need to make sure the file names are in sequential order.

highker self-requested a review

August 31, 2020 07:05

NikhilCollooru force-pushed the m2 branch from 5100427 to bdb099c Compare

September 2, 2020 00:01

highker reviewed

View reviewed changes

Contributor

highker left a comment

"Rename files written by HivePageSink": make rename async

presto-hive-common/src/main/java/com/facebook/presto/hive/filesystem/ExtendedFileSystem.java Outdated Show resolved Hide resolved

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

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

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

NikhilCollooru force-pushed the m2 branch 3 times, most recently from c77545f to 33d310c Compare

September 26, 2020 04:24

highker self-requested a review

September 27, 2020 03:02

highker reviewed

View reviewed changes

Contributor

highker left a comment

"POST the metadata results to worker": we need good ways to purge queries on coordinators

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

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

highker reviewed

View reviewed changes

Contributor

highker left a comment

"Add config parameters to enable hive file renaming": LGTM

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

highker reviewed

View reviewed changes

Contributor

highker left a comment

"Disable listFiles call to underlying storage": let's remove this commit in this PR. That is more like the standalone piece to review in the next PR

Contributor

highker commented Sep 27, 2020

First pass done. Feel free to re-request my reviews once the comments are addressed.

NikhilCollooru force-pushed the m2 branch 2 times, most recently from aec75b5 to eeb61e0 Compare

September 29, 2020 01:47

NikhilCollooru requested a review from highker

September 29, 2020 03:37

highker reviewed

View reviewed changes

Contributor

highker left a comment

Overall look good to me. Minor comments only

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

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

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

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

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

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveFileRenamer.java Outdated Show resolved Hide resolved

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveFileRenamer.java Outdated Show resolved Hide resolved

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java Outdated Show resolved Hide resolved

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java Show resolved Hide resolved

presto-main/src/main/java/com/facebook/presto/server/remotetask/TaskInfoFetcher.java Outdated Show resolved Hide resolved

NikhilCollooru force-pushed the m2 branch 2 times, most recently from 0ee1a54 to 6c40f9b Compare

October 1, 2020 02:13

NikhilCollooru requested a review from highker

October 1, 2020 03:31

highker approved these changes

View reviewed changes

Contributor

highker left a comment

good work! Nits only

presto-main/src/main/java/com/facebook/presto/execution/SqlTask.java Outdated Show resolved Hide resolved

presto-main/src/main/java/com/facebook/presto/execution/TaskManager.java Outdated Show resolved Hide resolved

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java Outdated Show resolved Hide resolved

highker self-assigned this

Nikhil Collooru added 6 commits

September 30, 2020 22:49


          Piggyback MetadataUpdateRequests to coordinator as part of TaskInfo

801401b

Collect all the metadata update requests and send them to coordinator
as part of the TaskInfo.


          Add new POST {taskId}/metadataresults endpoint on worker

ce16bc3


          Rename files written by HivePageSink

2d122fd


          POST the metadata results to worker

c137ae4


          Add config parameters to enable hive file renaming

1b377d2


          Add integration tests for file renaming

665045a

NikhilCollooru force-pushed the m2 branch from 6c40f9b to 665045a Compare

October 1, 2020 06:13

highker merged commit 4cd2141 into prestodb:master

NikhilCollooru deleted the m2 branch

October 1, 2020 20:38

This was referenced Oct 6, 2020

Add release notes for 0.242 #15270

Merged

[Test] Add release notes for 0.242 #15291

Closed

[Test-Only] Add release notes for 0.242 #15294

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet