[GitHub] [arrow-datafusion] andygrove commented on a change in pull request #232: Make external hostname in executor optional

[GitBox <gi...@apache.org>]

andygrove commented on a change in pull request #232:
URL: https://github.com/apache/arrow-datafusion/pull/232#discussion_r624544329



##########
File path: ballista/rust/executor/src/main.rs
##########
@@ -158,16 +166,22 @@ async fn main() -> Result<()> {
     let scheduler = SchedulerGrpcClient::connect(scheduler_url)
         .await
         .context("Could not connect to scheduler")?;
-    let executor = Arc::new(BallistaExecutor::new(config));
-    let service = BallistaFlightService::new(executor);
+    let service = BallistaFlightService::new(work_dir);
 
     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" {

Review 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?




-- 
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