-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add remote function server binary #6104
Conversation
✅ Deploy Preview for meta-velox canceled.
|
This pull request was exported from Phabricator. Differential Revision: D48328189 |
This pull request was exported from Phabricator. Differential Revision: D48328189 |
Summary: Pull Request resolved: facebookincubator#6104 Adding a binary target that can run and expose the test remote function server. This will be used by Presto end-to-end query tests. Differential Revision: D48328189 fbshipit-source-id: 6f2b95ba17344a5a61dc43736c9fc845417eb359
72d1536
to
0ef0143
Compare
This pull request was exported from Phabricator. Differential Revision: D48328189 |
Summary: Pull Request resolved: facebookincubator#6104 Adding a binary target that can run and expose the test remote function server. This will be used by Presto end-to-end query tests. Differential Revision: D48328189 fbshipit-source-id: 8ae635b070dd34f3a9eab83b791474c9dc84e3da
0ef0143
to
1bcb545
Compare
This pull request was exported from Phabricator. Differential Revision: D48328189 |
Summary: Pull Request resolved: facebookincubator#6104 Adding a binary target that can run and expose the test remote function server. This will be used by Presto end-to-end query tests. Also adding a couple of tweaks to the server handler implementation. Differential Revision: D48328189 fbshipit-source-id: 97a380075914af35600c1a5155870e2b0aefd7ba
1bcb545
to
090a5f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
using namespace ::facebook::velox; | ||
using ::apache::thrift::ThriftServer; | ||
|
||
int main(int argc, char* argv[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe comment on this executable and how its supposed to be invoked and that its pretty barebones right now etc intentionally (?)..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, will add. This is supposed to be pretty bare bones indeed and only used for testing. Basically we need a binary to run in CI and initialized the remote function server for Presto e2e tests.
I'll add some comments to clarify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If its for test purposes should it be inside a test dir ?
auto server = std::make_shared<ThriftServer>(); | ||
server->setInterface(handler); | ||
server->setAddress(location); | ||
server->serve(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe there should be a log after initialization saying initilzation succesful ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the thrift handler itself logs a few messages regarding its status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good to know !
This pull request was exported from Phabricator. Differential Revision: D48328189 |
Summary: Pull Request resolved: facebookincubator#6104 Adding a binary target that can run and expose the test remote function server. This will be used by Presto end-to-end query tests. Also adding a couple of tweaks to the server handler implementation. Reviewed By: kgpai Differential Revision: D48328189 fbshipit-source-id: b2c8a826382e161579b769f6ab2b006da292b938
090a5f1
to
b0f1ad8
Compare
Summary: Pull Request resolved: facebookincubator#6104 Adding a binary target that can run and expose the test remote function server. This will be used by Presto end-to-end query tests. Also adding a couple of tweaks to the server handler implementation. Reviewed By: kgpai Differential Revision: D48328189 fbshipit-source-id: bc1e16f9ca5c57b9d98de36248c5f1f8d67f5bf4
This pull request was exported from Phabricator. Differential Revision: D48328189 |
b0f1ad8
to
085fe42
Compare
This pull request has been merged in f59979e. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Summary: Pull Request resolved: facebookincubator#6104 Adding a binary target that can run and expose the test remote function server. This will be used by Presto end-to-end query tests. Also adding a couple of tweaks to the server handler implementation. Reviewed By: kgpai Differential Revision: D48328189 fbshipit-source-id: 7fbfa7cbc458fba6945e06e67dbb8394d0a13412
Summary: Pull Request resolved: facebookincubator#6104 Adding a binary target that can run and expose the test remote function server. This will be used by Presto end-to-end query tests. Also adding a couple of tweaks to the server handler implementation. Reviewed By: kgpai Differential Revision: D48328189 fbshipit-source-id: 7fbfa7cbc458fba6945e06e67dbb8394d0a13412
Summary:
Adding a binary target that can run and expose the test remote
function server. This will be used by Presto end-to-end query tests.
Differential Revision: D48328189