You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/11 18:45:39 UTC
[GitHub] [arrow] msathis opened a new pull request #9987: Add simple api server in scheduler
msathis opened a new pull request #9987:
URL: https://github.com/apache/arrow/pull/9987
Implements GET /executors. We can additional endpoints going forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove closed pull request #9987: ARROW-12332: [Rust] [Ballista] Add simple api server in scheduler
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #9987:
URL: https://github.com/apache/arrow/pull/9987
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Cheappie commented on pull request #9987: ARROW-12332: [Rust] [Ballista] Add simple api server in scheduler
Posted by GitBox <gi...@apache.org>.
Cheappie commented on pull request #9987:
URL: https://github.com/apache/arrow/pull/9987#issuecomment-817771050
Hi guys, I don't know how many endpoints you plan to have in future, but did you consider having additional prefix in path like `/scheduler/executors` ? ( FYI, I am just random person that is reading github issues )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #9987: ARROW-12332: [Rust] [Ballista] Add simple api server in scheduler
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #9987:
URL: https://github.com/apache/arrow/pull/9987#issuecomment-818730158
> Hi guys, I don't know how many endpoints you plan to have in future, but did you consider having additional prefix in path like `/scheduler/executors` ? ( FYI, I am just random person that is reading github issues )
It's a good point but we're so early with this feature it is hard to know how it will evolve. The process is a dedicated scheduler so adding a scheduler prefix seems redundant at this point though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #9987: ARROW-12332: [Rust] [Ballista] Add simple api server in scheduler
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #9987:
URL: https://github.com/apache/arrow/pull/9987#issuecomment-817401449
Hi @msathis and thanks for creating the PR. I pulled your branch and tried testing this locally but couldn't get it to work. I'm probably doing something wrong. It would be good if you could add some documentation as part of this PR. Perhaps provide a `curl` command for hitting this API?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9987: ARROW-12332: [Rust] Add simple api server in scheduler
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9987:
URL: https://github.com/apache/arrow/pull/9987#issuecomment-817354732
https://issues.apache.org/jira/browse/ARROW-12332
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] msathis edited a comment on pull request #9987: ARROW-12332: [Rust] Add simple api server in scheduler
Posted by GitBox <gi...@apache.org>.
msathis edited a comment on pull request #9987:
URL: https://github.com/apache/arrow/pull/9987#issuecomment-817354924
<img width="1196" alt="Screenshot 2021-04-12 at 12 22 21 AM" src="https://user-images.githubusercontent.com/2977899/114317329-2a421080-9b25-11eb-90ae-4161476b3d8c.png">
@andygrove Please review when you find time :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9987: Add simple api server in scheduler
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9987:
URL: https://github.com/apache/arrow/pull/9987#issuecomment-817354014
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/master/.github/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] msathis commented on pull request #9987: ARROW-12332: [Rust] [Ballista] Add simple api server in scheduler
Posted by GitBox <gi...@apache.org>.
msathis commented on pull request #9987:
URL: https://github.com/apache/arrow/pull/9987#issuecomment-817460796
```
curl --request GET \
--url http://localhost:50050/executors \
--header 'Accept: application/json'
```
@andygrove The above curl should work. Added small documentation to the PR as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] msathis commented on pull request #9987: ARROW-12332: [Rust] [Ballista] Add simple api server in scheduler
Posted by GitBox <gi...@apache.org>.
msathis commented on pull request #9987:
URL: https://github.com/apache/arrow/pull/9987#issuecomment-817964590
@Cheappie I don't think we need the prefix since the api is part of the scheduler. But my knowledge is limited.
@andygrove Do you foresee a requirement to support additional resources not related to schedulers in future?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] edrevo commented on a change in pull request #9987: ARROW-12332: [Rust] [Ballista] Add simple api server in scheduler
Posted by GitBox <gi...@apache.org>.
edrevo commented on a change in pull request #9987:
URL: https://github.com/apache/arrow/pull/9987#discussion_r624519673
##########
File path: rust/ballista/rust/scheduler/src/main.rs
##########
@@ -56,11 +61,35 @@ async fn start_server(
"Ballista v{} Scheduler listening on {:?}",
BALLISTA_VERSION, addr
);
- let server =
- SchedulerGrpcServer::new(SchedulerServer::new(config_backend, namespace));
- Ok(Server::builder()
- .add_service(server)
- .serve(addr)
+ Ok(Server::bind(&addr)
+ .serve(make_service_fn(move |_| {
+ let scheduler_server = SchedulerServer::new(config_backend.clone(), namespace.clone());
+ let scheduler_grpc_server = SchedulerGrpcServer::new(scheduler_server.clone());
+
+ let mut tonic = TonicServer::builder()
+ .add_service(scheduler_grpc_server)
+ .into_service();
+ let mut warp = warp::service(get_routes(scheduler_server));
+
+ future::ok::<_, Infallible>(tower::service_fn(
+ move |req: hyper::Request<hyper::Body>| {
+ let header = req.headers().get(hyper::header::ACCEPT);
+ if header.is_some() && header.unwrap().eq("application/json") {
+ return Either::Left(
+ warp.call(req)
+ .map_ok(|res| res.map(EitherBody::Left))
+ .map_err(Error::from),
+ );
+ }
+ Either::Right(
+ tonic
+ .call(req)
+ .map_ok(|res| res.map(EitherBody::Right))
+ .map_err(Error::from),
+ )
+ },
+ ))
Review comment:
Sorry I'm super late to this PR. Does this change mean that a SchedulerGrpcServer is being built for every request?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] msathis commented on pull request #9987: ARROW-12332: [Rust] Add simple api server in scheduler
Posted by GitBox <gi...@apache.org>.
msathis commented on pull request #9987:
URL: https://github.com/apache/arrow/pull/9987#issuecomment-817354924
<img width="1196" alt="Screenshot 2021-04-12 at 12 22 21 AM" src="https://user-images.githubusercontent.com/2977899/114317329-2a421080-9b25-11eb-90ae-4161476b3d8c.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] msathis commented on pull request #9987: ARROW-12332: [Rust] [Ballista] Add simple api server in scheduler
Posted by GitBox <gi...@apache.org>.
msathis commented on pull request #9987:
URL: https://github.com/apache/arrow/pull/9987#issuecomment-818713766
@andygrove Can you please get it merged, if everything is okay?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org