Make external hostname in executor optional #232
Conversation
|
cc @andygrove |
| @@ -62,17 +62,20 @@ async fn start_server( | |||
| BALLISTA_VERSION, addr | |||
| ); | |||
|
|
|||
| let scheduler_server = | |||
| SchedulerServer::new(config_backend.clone(), namespace.clone()); | |||
| Ok(Server::bind(&addr) | |||
There was a problem hiding this comment.
I'm a bit concerned this code is creating a new server per client. See https://github.com/apache/arrow/pull/9987/files#r624519673
Codecov Report
@@ Coverage Diff @@
## master #232 +/- ##
==========================================
+ Coverage 76.46% 76.70% +0.24%
==========================================
Files 135 134 -1
Lines 23250 23174 -76
==========================================
- Hits 17777 17776 -1
+ Misses 5473 5398 -75
Continue to review full report at Codecov.
|
|
|
||
| let server = FlightServiceServer::new(service); | ||
| info!( | ||
| "Ballista v{} Rust Executor listening on {:?}", | ||
| BALLISTA_VERSION, addr | ||
| ); | ||
| let server_future = tokio::spawn(Server::builder().add_service(server).serve(addr)); | ||
| let client = BallistaClient::try_new(&external_host, port).await?; | ||
| let client_host = external_host.as_deref().unwrap_or_else(|| { | ||
| if bind_host == "0.0.0.0" { |
There was a problem hiding this comment.
I'm not sure I understand what the intent is here. According to https://en.wikipedia.org/wiki/0.0.0.0, binding to 0.0.0.0 means binding to "any IPv4 address at all". This seems to change that behavior and prevents the user from doing that. Perhaps you could add some documentation here to explain this?
There was a problem hiding this comment.
I have added a comment to clarify. Right now the executor does a really weird thing which has a big TODO here:
Basically, the executor needs to connect to itself through a BallistaClient in order to work. If there is an external host defined, then it is clear how to connect to oneself. If not, we need to check the bind address, but since 0.0.0.0 is a meta-address, for that case we can just use localhost to connect to ourselves. Does that make sense?
I started working on the TODO to get rid of this ugliness, but then the PR would have been too big, so I was planning on tackling that separately.
|
Thanks @edrevo this looks like a good cleanup. I will find time this weekend (likely tomorrow) to pull this PR and do some testing to make sure I understand everything that is happening here. |
There was a problem hiding this comment.
Thanks, @edrevo. I pulled the branch locally and tested running scheduler and executor on separate computers on the same network (in standalone mode - no containerization) and saw the executor register with the scheduler. I didn't run any queries because I'm not set up to be able to do that easily yet.
I like the ability to scale more easily in docker-compose 💯
houqp
added
api change
Which issue does this PR close?
Closes #76.
Rationale for this change
From the issue:
What changes are included in this PR?
Are there any user-facing changes?
Advertised hostname is now optional when launching the executor.