andygrove commented on PR #2416: URL: https://github.com/apache/datafusion-ballista/pull/2416#issuecomment-5858146294
Here's where the rest of the review summary landed. **Missing tests.** The stub backend now returns three partitions, and `select_produces_one_endpoint_per_partition_with_no_location` checks that each endpoint names a different one. `an_authenticator_makes_tokenless_requests_fail` covers `DoGet` and the prepared statement handlers. `EXECUTE` is in `statements_that_cannot_be_distributed_are_refused`. The `PushStaged` test went to #2492 with the fix. **Duplicate tests.** `the_default_service_allows_anonymous_access` is now one assertion in the authenticator test. The `GetTables` test in `tests/service.rs` is gone, so `GetTables` is covered once in `metadata.rs` and once end to end. `sql_info_is_buildable`, `xdbc_type_info_is_buildable` and `plain_ddl_runs_on_the_scheduler` are removed. `local_results_are_single_use` folded into the session binding test, and the two bearer token tests are one. **Comments.** All four are fixed. `LocalResult` no longer mentions DML. `query_failed` no longer claims to avoid a 500. The `QueryBackend::session` doc says the frontend caches the context, so a backend may build a fresh one on every call. The notes about the pre-46.0.0 implementation are out of the code and tests. **Robustness.** The reaper starts on the first request, so the service can be built outside a Tokio runtime. A handshake no longer builds a `SessionContext`. The context is built on first use. I haven't added count caps. Everything in the store expires after the 30 minute idle TTL, and local results are dropped as soon as they're redeemed. The same port also lets any client create scheduler sessions with no cap through `SchedulerGrpc::create_update_session`. So a cap here wouldn't bound memory against a hostile client, which is why the docs say to keep the port on a trusted network. I'm open to adding caps if you think they're worth it for runaway clients. **Performance.** Agreed, this is a real limit. Endpoints carry no location so clients never need to reach an executor (#1012, #1349), which makes the scheduler the funnel for result bytes. The per-`DoGet` connection is how the existing `BallistaFlightProxyService` works, and this reuses it. Pooling connections in the proxy, or an opt-in to advertise executor locations to clients that can reach them, would both help. I'd like to leave those for a follow-up with a benchmark that uses large results. The TPC-H runner in #2496 times `DoGet` separately, but TPC-H answers are too small to show 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
