Add optional flag which advertises host for Arrow Flight SQL #418 #442

DaltonModlin · 2022-10-24T19:44:05Z

Update scheduler_config_spec.toml to include new flag 'advertise_host'
Add advertise_host member variable to SchedulerServer
Add advertise_host argument to new, new_with_policy, new_with_builder, and new_with_state in order to propagate flag
Add None argument to relevant method calls

Add optional flag which advertises host for Arrow Flight SQL #418

Update logic in job_to_fetch_part to use advertise-host flag when it exists
Remove default from advertise_host in scheduler_config_spec.toml
Wrap scheduler_server advertise_host variable in Option
Update scheduler's main.rs to reflect advertise_host being wrapped in Option

Utilize executor IP for routing to flight results in job_to_fetch_part even when advertise-host flag is set.

Add missing variable and ownership stuff

Remove unnecessary output from do_get_fallback

Which issue does this PR close?

Closes #418

Rationale for this change

What changes are included in this PR?

Adds member variable to SchedulerServer, adds argument to relevant constructors as necessary.

Are there any user-facing changes?

Adds advertise-host flag for proxying via scheduler as described in original issue.

DaltonModlin · 2022-10-24T20:49:59Z

I've validated my changes have the expected outcome by writing a test in the flight portion of the arrow repo.

@Test
public void testBallistaFlag() throws Exception {
    String url = "jdbc:arrow-flight://127.0.0.0:50050";
    java.util.Properties props = new java.util.Properties();
    props.setProperty("useEncryption", "false");
    props.setProperty("user", "admin");
    props.setProperty("password", "password");
    Connection con = DriverManager.getConnection(url, props);
    java.sql.Statement stmt = con.createStatement();
    String sql = "create external table customer STORED AS CSV WITH HEADER ROW LOCATION 'arrow-datafusion/datafusion/core/tests/tpch-csv/customer.csv';\n";
    assertTrue(stmt.execute(sql));
}

Running this test in conjunction with a local scheduler (having passing it --advertise-host="127.0.0.2:50050") and a local executor shows the request for flight results is proxied via the Scheduler rather than going directly to the executor IP.

avantgardnerio · 2022-10-24T20:57:29Z

flight results is proxied

I'm actually a little surprised that query returns a flight since it is DML.

@andygrove I think we'll want to merge this PR because we presently rely on a bug in the FlightSQL JDBC driver where Ballista only works because it always re-uses the connection to the scheduler. This PR should make it work even after that issue is fixed.

avantgardnerio

I'd say the unwraps and unsafe array indexing need resolution. The others are just nits.

Nice job! 🥳

avantgardnerio · 2022-10-24T20:59:03Z

ballista/scheduler/src/flight_sql.rs

+                        .server
+                        .advertise_host
+                        .as_ref()
+                        .unwrap()


I think we want to avoid unwrap()s as a rule, but especially in functions that already return Results...

avantgardnerio · 2022-10-24T21:01:11Z

ballista/scheduler/src/flight_sql.rs

+                        .unwrap()
+                        .split(":")
+                        .collect();
+                    (advertise_host_flag[0].to_string(), advertise_host_flag[1].parse().unwrap())


Same... I think there is a safe way to index into the array: https://adventures.michaelfbryan.com/posts/daily/slice-patterns/

ballista/scheduler/scheduler_config_spec.toml

avantgardnerio

Sorry, accidentally marked it approve. I'd like to see the unwraps fixed.

andygrove · 2022-10-25T13:49:03Z

Approach LGTM. Agree on the unwraps. I would like to test this out as well before approving. I'll try and get that to that today.

ballista/scheduler/src/flight_sql.rs

andygrove

LGTM. Thanks @DaltonModlin!

avantgardnerio · 2022-10-26T22:02:18Z

ballista/scheduler/src/flight_sql.rs

+                Some(endpoint) => {
+                    let advertise_endpoint_vec: Vec<&str> = endpoint.split(":").collect();
+                    match advertise_endpoint_vec.as_slice() {
+                        [host_ip, port] => {


andygrove · 2022-10-26T22:32:01Z

@DaltonModlin There is a clippy failure .. I will merge once that is resolved

DaltonModlin · 2022-10-27T18:27:38Z

It looks like clippy is worried because there are 8 argument to the function new_with_policy. It might be worth using a Builder pattern to construct the SchedulerServer rather than the current pattern of many slightly differing constructors.

andygrove · 2022-10-31T19:18:45Z

@DaltonModlin Looks like there is conflict?

- Update scheduler_config_spec.toml to include new flag 'advertise_host' - Add advertise_host member variable to SchedulerServer - Add advertise_host argument to new, new_with_policy, new_with_builder, and new_with_state in order to propagate flag - Add None argument to relevant method calls Add optional flag which advertises host for Arrow Flight SQL apache#418 - Update logic in job_to_fetch_part to use advertise-host flag when it exists - Remove default from advertise_host in scheduler_config_spec.toml - Wrap scheduler_server advertise_host variable in Option - Update scheduler's main.rs to reflect advertise_host being wrapped in Option Utilize executor IP for routing to flight results in job_to_fetch_part even when advertise-host flag is set. Add missing variable and ownership stuff Remove unnecessary output from do_get_fallback Responding to PR feedback - Rename advertise-host to advertise-endpoint - Replace unwrap calls with expect where possible - Add missing error handling when parsing advertise-endpoint flag PR feedback Co-authored-by: Andy Grove <[email protected]> PR Feedback - Using slice rather than array indexing for parsing advertise-endpoint PR Feedback - Fix clippy issue Fix pipeline failure

DaltonModlin marked this pull request as ready for review October 24, 2022 19:48

avantgardnerio approved these changes Oct 24, 2022

View reviewed changes

avantgardnerio requested changes Oct 24, 2022

View reviewed changes

andygrove reviewed Oct 26, 2022

View reviewed changes

ballista/scheduler/src/flight_sql.rs Outdated Show resolved Hide resolved

avantgardnerio approved these changes Oct 26, 2022

View reviewed changes

andygrove approved these changes Oct 26, 2022

View reviewed changes

avantgardnerio reviewed Oct 26, 2022

View reviewed changes

DaltonModlin force-pushed the dalto_418 branch from 58fdd34 to f0d1f5b Compare October 31, 2022 18:51

DaltonModlin force-pushed the dalto_418 branch from f0d1f5b to 4d93ed7 Compare October 31, 2022 19:35

DaltonModlin force-pushed the dalto_418 branch from 4d93ed7 to 4456e02 Compare October 31, 2022 20:27

andygrove merged commit 7aa2f85 into apache:master Oct 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional flag which advertises host for Arrow Flight SQL #418 #442

Add optional flag which advertises host for Arrow Flight SQL #418 #442

DaltonModlin commented Oct 24, 2022 •

edited

Loading

DaltonModlin commented Oct 24, 2022 •

edited

Loading

avantgardnerio commented Oct 24, 2022

avantgardnerio left a comment

avantgardnerio Oct 24, 2022

avantgardnerio Oct 24, 2022

avantgardnerio left a comment

andygrove commented Oct 25, 2022

andygrove left a comment

avantgardnerio Oct 26, 2022

andygrove commented Oct 26, 2022

DaltonModlin commented Oct 27, 2022

andygrove commented Oct 31, 2022

Add optional flag which advertises host for Arrow Flight SQL #418 #442

Add optional flag which advertises host for Arrow Flight SQL #418 #442

Conversation

DaltonModlin commented Oct 24, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

DaltonModlin commented Oct 24, 2022 • edited Loading

avantgardnerio commented Oct 24, 2022

avantgardnerio left a comment

Choose a reason for hiding this comment

avantgardnerio Oct 24, 2022

Choose a reason for hiding this comment

avantgardnerio Oct 24, 2022

Choose a reason for hiding this comment

avantgardnerio left a comment

Choose a reason for hiding this comment

andygrove commented Oct 25, 2022

andygrove left a comment

Choose a reason for hiding this comment

avantgardnerio Oct 26, 2022

Choose a reason for hiding this comment

andygrove commented Oct 26, 2022

DaltonModlin commented Oct 27, 2022

andygrove commented Oct 31, 2022

DaltonModlin commented Oct 24, 2022 •

edited

Loading

DaltonModlin commented Oct 24, 2022 •

edited

Loading