Skip to content

[Presto-on-Spark] Move DetachedNativeExecutionProcess from test to src#19435

Merged
shrinidhijoshi merged 1 commit intoprestodb:masterfrom
shrinidhijoshi:localMode-4
May 3, 2023
Merged

[Presto-on-Spark] Move DetachedNativeExecutionProcess from test to src#19435
shrinidhijoshi merged 1 commit intoprestodb:masterfrom
shrinidhijoshi:localMode-4

Conversation

@shrinidhijoshi
Copy link
Collaborator

@shrinidhijoshi shrinidhijoshi commented Apr 19, 2023

Currently DetachedNativeExecutionProcess class is defined in test folder as it was only being used for test execution. But now that we want to support Presto-on-Spark in localMode, we want to be able to talk to an already running CPP process during e2e query execution. Moving it out into src to enable this

Test plan (N/A)

  • Ensure all current tests that use DetachedNativeExecutionProcess pass
== NO RELEASE NOTE ==

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch @miaoever . Looks like there are a few things we can clean up as a part of this. Will update

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what line it is as that class is no longer there. But things have changed recently so we should double check. Thanks

@tanjialiang
Copy link
Contributor

High level question: What is the setup for local mode and why do we need a detached process for local mode? Thanks!

@shrinidhijoshi
Copy link
Collaborator Author

shrinidhijoshi commented Apr 25, 2023

@tanjialiang There 2 types of localMode

  1. With standalone cpp process (on devserver)(This PR) - This is needed when you want to start the CPP process in debug mode with breakpoints.

  2. With inbuilt cpp process (downloaded through fbpkg) mimicing production - This one is useful when you just want to debug the java side of the flow and do not care about setting up cpp instance on your devserver

Copy link
Contributor

@miaoever miaoever left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for adding this @shrinidhijoshi

@shrinidhijoshi
Copy link
Collaborator Author

Resolved merge conflicts

Currently DetachedNativeExecutionProcess class is defined in test folder
as it was only being used for test execution. But now that we want to
support Presto-on-Spark in localMode, we want to be able to talk
to an already running CPP process during e2e query execution. Moving it
out into src to enable this
@shrinidhijoshi
Copy link
Collaborator Author

Test failures are unrelated. I re-ran the tests on laptop and they pass

Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@shrinidhijoshi Please, make sure all CI is green.

@shrinidhijoshi
Copy link
Collaborator Author

@mbasmanova Will do. Kicked off the failing tests again

Copy link
Contributor

@ajaygeorge ajaygeorge left a comment

Choose a reason for hiding this comment

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

LGTM

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants