Skip to content
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.

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

refactor(interactive): Add error handling for adhoc queries #4188

Merged
merged 14 commits into from
Sep 6, 2024

Conversation

zhanglei1949
Copy link
Collaborator

@zhanglei1949 zhanglei1949 commented Aug 28, 2024

We Introduce boost::leaf for lightweight error handling in the Interactive Ad Hoc Query Service.
For each operator, we refactor it returns a bl::result value, allowing the error to be caught and returned at the most outside, i.e. adhoc_app.cc.

In this PR, we only wrap the errors about UNSUPPORTED UNIMPLEMENTED and BAD_REQUET at operator level. For lower level errors, we still let it crash.

TBD:

  • GAE depends on boost::leaf via vineyard, but we don't want to depend on vineyard, so a new submodule is introduced, is this ok?

Fix #4189

We will continue to resolve these UNSUPPORTED and UNIMPLEMENTED errors, by covering all combination of all params and operators in physical plan.
TODO #4190

@zhanglei1949 zhanglei1949 marked this pull request as draft August 28, 2024 02:31
@@ -20,13 +20,6 @@ namespace gs {
static unsigned long long lastTotalUser, lastTotalUserLow, lastTotalSys,
lastTotalIdle;

FlexException::FlexException(std::string&& error_msg)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That code is no longer used.

Copy link
Contributor

github-actions bot commented Aug 28, 2024

Please check the preview of the documentation changes at
https://78e97e4c.graphscope-docs-preview.pages.dev

@zhanglei1949 zhanglei1949 marked this pull request as ready for review August 28, 2024 07:47
@zhanglei1949
Copy link
Collaborator Author

@yecol @sighingnow @siyuan0322 Is there any better way to introduce boost::leaf for Interactive? Ensure it is compatible with the dependency structure of vineyard and GAE.

siyuan0322
siyuan0322 previously approved these changes Aug 29, 2024
@siyuan0322
Copy link
Collaborator

@yecol @sighingnow @siyuan0322 Is there any better way to introduce boost::leaf for Interactive? Ensure it is compatible with the dependency structure of vineyard and GAE.

If the development container and builder dockerfile is shared with analytical engine, then boost::leaf is already installed. So maybe you could find boost::leaf in system path first in CMake?

If its for users to install manually, then installing boost::leaf could be a prerequisite.

@zhanglei1949
Copy link
Collaborator Author

zhanglei1949 commented Aug 29, 2024

@yecol @sighingnow @siyuan0322 Is there any better way to introduce boost::leaf for Interactive? Ensure it is compatible with the dependency structure of vineyard and GAE.

If the development container and builder dockerfile is shared with analytical engine, then boost::leaf is already installed. So maybe you could find boost::leaf in system path first in CMake?

If its for users to install manually, then installing boost::leaf could be a prerequisite.

I got it. But one thing is unclear to me - even though leaf.hpp is installed by vineyard in the graphscope-dev container and the base image of the graphscope interactive, the path is relative to where vineyard is installed, specifically ${VINEYARD_INSTALL_PREFIX}/include/vineyard/contrib/boost. Without find_package(vineyard), we cannot determine the installed prefix of vineyard.

@yecol
Copy link
Collaborator

yecol commented Aug 29, 2024

Please comfirm with Dongze: .graphscope_env should address the paths issues.

@siyuan0322
Copy link
Collaborator

@yecol @sighingnow @siyuan0322 Is there any better way to introduce boost::leaf for Interactive? Ensure it is compatible with the dependency structure of vineyard and GAE.

If the development container and builder dockerfile is shared with analytical engine, then boost::leaf is already installed. So maybe you could find boost::leaf in system path first in CMake?
If its for users to install manually, then installing boost::leaf could be a prerequisite.

I got it. But one thing is unclear to me - even though leaf.hpp is installed by vineyard in the graphscope-dev container and the base image of the graphscope interactive, the path is relative to where vineyard is installed, specifically ${VINEYARD_INSTALL_PREFIX}/include/vineyard/contrib/boost. Without find_package(vineyard), we cannot determine the installed prefix of vineyard.

We could do a link in the dockerfile.

@lidongze0629
Copy link
Collaborator

@zhanglei1949 @dashanji Vineyard does rely on boost leaf as a thirdparty and install to /opt/vineyard/include/vineyard/contrib/boost/leaf directory. I suggest we could install boost leaf in global, and both vineyard and interactive try to use it first;

@dashanji
Copy link
Collaborator

@zhanglei1949 @dashanji Vineyard does rely on boost leaf as a thirdparty and install to /opt/vineyard/include/vineyard/contrib/boost/leaf directory. I suggest we could install boost leaf in global, and both vineyard and interactive try to use it first;

Yeah, vineyard will find the boost leaf package first. If not exist, then use the thirdparty code.
https://github.com/v6d-io/v6d/blob/main/modules/graph/CMakeLists.txt#L85-L102

I think interactive could do the same.

@zhanglei1949
Copy link
Collaborator Author

zhanglei1949 commented Aug 29, 2024

@zhanglei1949 @dashanji Vineyard does rely on boost leaf as a thirdparty and install to /opt/vineyard/include/vineyard/contrib/boost/leaf directory. I suggest we could install boost leaf in global, and both vineyard and interactive try to use it first;

Yeah, vineyard will find the boost leaf package first. If not exist, then use the thirdparty code. https://github.com/v6d-io/v6d/blob/main/modules/graph/CMakeLists.txt#L85-L102

I think interactive could do the same.

Great! So correct me if I'm wrong:

  1. First we need boost leaf installed globally in graphscope-dev image. And also gsctl should be able to install boost leaf globally, when installing dependencies for Flex Interactive(or for other components)
  2. Interactive will expect a boost leaf package installed globally. If that fails, interactive will tell the user to install boost leaf. (Although we may try to find available boost/leaf.hpp somewhere, from my point of view, it would be somehow adhoc.)
  3. Interactive will NOT introduce boost leaf as a third party dependency.

@lidongze0629 @dashanji @luoxiaojian @yecol

…ueries

Committed-by: xiaolei.zl from Dev container

Committed-by: xiaolei.zl from Dev container
Committed-by: xiaolei.zl from Dev container

Committed-by: xiaolei.zl from Dev container
Committed-by: xiaolei.zl from Dev container

Committed-by: xiaolei.zl from Dev container
Committed-by: xiaolei.zl from Dev container

Committed-by: xiaolei.zl from Dev container
Committed-by: xiaolei.zl from Dev container
Committed-by: xiaolei.zl from Dev container

Committed-by: xiaolei.zl from Dev container
Committed-by: xiaolei.zl from Dev container

Committed-by: xiaolei.zl from Dev container
Committed-by: xiaolei.zl from Dev container
liulx20
liulx20 previously approved these changes Sep 5, 2024
Committed-by: xiaolei.zl from Dev container
siyuan0322
siyuan0322 previously approved these changes Sep 5, 2024
Committed-by: xiaolei.zl from Dev container
Committed-by: xiaolei.zl from Dev container
Committed-by: xiaolei.zl from Dev container
Committed-by: xiaolei.zl from Dev container
Committed-by: xiaolei.zl from Dev container
@zhanglei1949 zhanglei1949 merged commit 2397f8a into alibaba:main Sep 6, 2024
13 checks passed
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.

Add error handling for interactive adhoc query
6 participants