[native pos] Change from Operator to Task Level Adapter by shrinidhijoshi · Pull Request #19799 · prestodb/presto

shrinidhijoshi · 2023-06-06T05:39:19Z

Summary

This PR introduces below changes

Add - PrestoSparkNativeTaskExecutorFactory singleton class (executor lifetime). This class is responsible for executing PrestoSparkNativeTaskRDDs.
Remove - NativeExecutionNode, NativeExecutionOperator , NativeExecutionInfo and corresponding plan rewrites and special handling
Rework Spark Metrics Integration for new adapter

Design

The new PrestoSparkNativeTaskExecutorFactory does below steps

Create the CPP process once when the first task arrives.
For every task that arrives
- Forward the task descriptors to the CPP process for execution. as BatchTaskUpdateInfo object
- Return a Result Iterator to Spark layer, which
  - Wait on pages or taskCompletion from the CPP process
  - Handle any exceptions during task processing
  - If task is complete, populate TaskInfoAccumulator to propagate statistics

Implementation notes/caveats

The current implementation does not introduce any new quirks / caveats. Rather exposes some from the old adapter

Piggyback on the existing classes for process management (NativeExecutionProcess, NativeExecutionTask, NativeExecutionProcessFactory, etc)
Usage of a dummyRemoteSourceTaskId when generating shuffle read splits. More details in comments

Test Plan

== NO RELEASE NOTE ==

mbasmanova

@shrinidhijoshi Love this change. Skimmed the first commit and make some minor comments. Overall, I find this change well written with nice documentation and easy to follow code. Thank you for taking the time to put this in a such beautiful shape.

mbasmanova · 2023-06-09T11:34:57Z

...se/src/main/java/com/facebook/presto/spark/execution/HttpNativeExecutionTaskInfoFetcher.java

What is this change about? Seems unrelated.

While this class was being used by NativeExecutionOperator, it was assumed that getTaskInfo will always be called while task is running.
The way the new PrestoSparkNativeTaskOutputIterator is implemented, that is not the case. We stop() the task first and then do a final query to get TaskInfo for statistics (check completeTask function), in which case we need to check if there was an exception before throwing.

I can change that implementation to extract TaskInfo before stopping, but it seemed like improving this check was a better approach.

Not leaning strongly either way, let me know if you have a preference

mbasmanova · 2023-06-09T11:35:11Z

presto-spark-base/src/main/java/com/facebook/presto/spark/execution/NativeExecutionProcess.java

add empty line

mbasmanova · 2023-06-09T11:35:47Z

...rk-base/src/main/java/com/facebook/presto/spark/execution/NativeExecutionProcessFactory.java

Should this not include a port?

The port is selected dynamically by the NativeExecutionProcess/DetachedNativeExecutionProcess so we don't need to specify it here.

presto-spark-base/src/test/java/com/facebook/presto/spark/PrestoSparkQueryRunner.java

.../src/main/java/com/facebook/presto/spark/execution/PrestoSparkNativeTaskExecutorFactory.java

mbasmanova · 2023-06-09T11:41:23Z

.../src/main/java/com/facebook/presto/spark/execution/PrestoSparkNativeTaskExecutorFactory.java

This is quite unfortunate. But, why do we need every executor to log plan fragment? Shouldn't we have driver log these once?

PlanFragment log in executor aids debugging to easily spot which fragment is being executed. Also, given internally at Meta, executors logs always contains logs of ALL the tasks run on that executor v/s just that task, it is hard to visually draw boundaries on these logs without plan fragment logging.

.../src/main/java/com/facebook/presto/spark/execution/PrestoSparkNativeTaskExecutorFactory.java

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

mbasmanova

@shrinidhijoshi Very happy to see this change. Let's get this in.

Add a new class PrestoSparkNativeTaskExecutorFactory which is responsible for executing native task remotely. It contains logic to start a native task, monitor for results and handle any exceptions that occur during task execution This is a singleton class that is selectively invoked only while executing a PrestoSparkNativeTaskRDD

Currently, for native execution, we re-write the plan fragment to add a NativeExecutionNode as the root to encapsulate the execution of native task. We do not need this anymore as the newly added PrestoSparkNativeTaskExecutorFactory will transparently forward the PlanFragment as is to CPP process

…xecutorFactory Current integration of metrics for presto-spark-native into spark metrics relies on extracting Operator level info from TaskInfo objects from Native Process. After the introduction of PrestoSparkNativeTaskExecutorFactory, we no longer use NativeExecutionOperator. So instead we can directly extract metrics from TaskInfo object.

Currently, there are no usages of NativeExecutionNode and NativeExecutionOperator after the introduction of PrestoSparkNativeTaskExecutorFactory. This commit removes them and other dormant/unused code that handles these classes.

We no longer use NativeExecutionInfo as we have all the task stats captured correctly as part of TaskInfo objects returned from Cpp process. This commit removes this class

This helps provide a hook for PrestoSparkNativeTaskExecutoryFactory to shutdown the native process

shrinidhijoshi requested review from a team as code owners June 6, 2023 05:39

shrinidhijoshi requested a review from ajaygeorge June 6, 2023 05:39

shrinidhijoshi force-pushed the ThinJavaExecutor-2 branch 2 times, most recently from 32877cc to 7736270 Compare June 6, 2023 05:53

shrinidhijoshi changed the title ~~[Presto-on-Spark] Add PrestoSparkNativeTaskExecutorFactory for Native Task Execution~~ [WIP do not review] [Presto-on-Spark] Add PrestoSparkNativeTaskExecutorFactory for Native Task Execution Jun 6, 2023

shrinidhijoshi force-pushed the ThinJavaExecutor-2 branch 14 times, most recently from bf47c9d to ae965eb Compare June 8, 2023 20:59

shrinidhijoshi changed the title ~~[WIP do not review] [Presto-on-Spark] Add PrestoSparkNativeTaskExecutorFactory for Native Task Execution~~ [Presto-on-Spark native] Implement TaskLevel Adapter for Presto-on-Spark Native Jun 8, 2023

shrinidhijoshi changed the title ~~[Presto-on-Spark native] Implement TaskLevel Adapter for Presto-on-Spark Native~~ [Presto-on-Spark native] Change from Operator to Task Level Adapter for Presto-on-Spark Native Jun 8, 2023

shrinidhijoshi changed the title ~~[Presto-on-Spark native] Change from Operator to Task Level Adapter for Presto-on-Spark Native~~ [native pos] Change from Operator to Task Level Adapter Jun 8, 2023

shrinidhijoshi force-pushed the ThinJavaExecutor-2 branch 2 times, most recently from 044f699 to 095314c Compare June 8, 2023 23:11

mbasmanova reviewed Jun 9, 2023

View reviewed changes

presto-main/src/main/java/com/facebook/presto/sql/planner/BasePlanFragmenter.java Outdated Show resolved Hide resolved

mbasmanova approved these changes Jun 9, 2023

View reviewed changes

shrinidhijoshi force-pushed the ThinJavaExecutor-2 branch 2 times, most recently from 14fe76c to 5003d8d Compare June 10, 2023 00:36

shrinidhijoshi force-pushed the ThinJavaExecutor-2 branch 6 times, most recently from 7e23ec8 to 8227516 Compare June 11, 2023 04:05

shrinidhijoshi added 6 commits June 12, 2023 16:27

[native pos] Remove NativeExecutionInfo special handling

1186e5e

We no longer use NativeExecutionInfo as we have all the task stats captured correctly as part of TaskInfo objects returned from Cpp process. This commit removes this class

[native pos] Add close method to IPrestoSparkTaskExecutorFactory

b5cf219

This helps provide a hook for PrestoSparkNativeTaskExecutoryFactory to shutdown the native process

shrinidhijoshi force-pushed the ThinJavaExecutor-2 branch from 8227516 to b5cf219 Compare June 12, 2023 23:28

shrinidhijoshi merged commit fa07644 into prestodb:master Jun 13, 2023

rschlussel mentioned this pull request Jul 18, 2023

Add Shrinidhi as codeowner for presto-spark* modules #20331

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native pos] Change from Operator to Task Level Adapter#19799

[native pos] Change from Operator to Task Level Adapter#19799
shrinidhijoshi merged 6 commits intoprestodb:masterfrom
shrinidhijoshi:ThinJavaExecutor-2

shrinidhijoshi commented Jun 6, 2023 •

edited

Loading

Uh oh!

mbasmanova left a comment

Uh oh!

mbasmanova Jun 9, 2023

Uh oh!

shrinidhijoshi Jun 9, 2023 •

edited

Loading

Uh oh!

mbasmanova Jun 9, 2023

Uh oh!

mbasmanova Jun 9, 2023

Uh oh!

shrinidhijoshi Jun 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mbasmanova Jun 9, 2023

Uh oh!

shrinidhijoshi Jun 9, 2023

Uh oh!

Uh oh!

Uh oh!

mbasmanova left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

shrinidhijoshi commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Design

Implementation notes/caveats

Test Plan

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

mbasmanova Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

shrinidhijoshi Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbasmanova Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

mbasmanova Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

shrinidhijoshi Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mbasmanova Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

shrinidhijoshi Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shrinidhijoshi commented Jun 6, 2023 •

edited

Loading

shrinidhijoshi Jun 9, 2023 •

edited

Loading