apache / arrow-flight-sql-postgresql

Apache Arrow Flight SQL adapter for PostgreSQL
https://arrow.apache.org/flight-sql-postgresql/
Apache License 2.0
68 stars 9 forks source link

Add support for closing a database (session) #13

Open kou opened 1 year ago

kou commented 1 year ago

It seems that Apache Arrow Flight SQL doesn't provide a command that closes the current session explicitly: https://arrow.apache.org/docs/format/FlightSql.html

In https://lists.apache.org/thread/0w25o85y8vsndz87kpjljxz24x077o3y , using a custom (close) action was suggested.

@lidavidm Is using a custom close action still a suggested approach to close the current session? I have a concern about this approach. If each Apache Arrow Flight SQL server implementation uses different custom close action, users can't use a general Apache Arrow Flight SQL client including ADBC to use Apache Arrow Flight SQL servers. (Users need to implement a custom close action for each Apache Arrow Flight SQL server.)

lidavidm commented 1 year ago

I've been working on a set of new proposals for Flight SQL. I think we should address this as part of that.

I don't want us to enforce statefulness if possible, because that runs counter to how gRPC/Flight work. So the server should still be able to deal with clients that do not send the close action.

lidavidm commented 1 year ago

Actually, I think James's proposal already covers this fully: https://lists.apache.org/thread/fd6r1n7vt91sg2c7fr35wcrsqz6x4645

Would you like to follow up there?

kou commented 1 year ago

Thanks! I missed the discussion. I'll reply on the thread.

kou commented 1 year ago

Note: Session timeout instead of closing session explicitly is implemented for now.

zhjwpku commented 1 year ago

I tried this extension and noticed that each executor will last for 5 minutes due to session timeout, I glanced the FlightSqlServerBase class a little bit, if it has a method knows the client's close action, then we can use it to close the executor. Is there any capability in arrow flight that we can use now?

kou commented 1 year ago

No there isn't. We need to forward the discussion referred at https://github.com/apache/arrow-flight-sql-postgresql/issues/13#issuecomment-1424289284 for it. Could you join the discussion on the dev@arrow.apache.org mailing list?

kou commented 1 year ago

Ah, https://github.com/apache/arrow/pull/34817 is a proposed implementation. I thought that it's not related to https://github.com/apache/arrow-flight-sql-postgresql/issues/13#issuecomment-1424289284 discussion.

cisaacson commented 1 month ago

I am interested in this capability for the Rust version of arrow-flight-sql. A timeout would not meet our requirements. I would suggest some type of on_close event that the client calls on the server as it is closing. If I can help please let me know.

kou commented 1 month ago

https://github.com/apache/arrow/pull/34817 has been merged. So we can implement https://arrow.apache.org/docs/format/FlightSql.html#flight-server-session-management for explicit close.

cisaacson commented 1 month ago

Thanks @kou, the CloseSession command is exactly what I was looking for. I have 2 questions:

If I can help with an example I would be happy to do that, for implementing the Rust version it may be better for someone more familiar with the codebase than me.

And thanks for all the work on this.

cisaacson commented 1 month ago

I don't know if this is the right place to report this, but the latest version of master for arrow-rs does not compile:

no associated item named `GRPC_STATUS` found for struct `Status` in the current scope
    --> arrow-flight/src/arrow.flight.protocol.rs:1578:48

If I should post it somewhere else let me know.

kou commented 1 month ago

We need the followings for explicit close:

Could you report arrow-rs error to https://github.com/apache/arrow-rs/issues ?

cisaacson commented 1 month ago

@kou Sure, I will report it there.

cisaacson commented 1 month ago

This issue can be closed as it has been reported in the arrow-rs project.

kou commented 1 month ago

This issue isn't for arrow-rs. So we should not close this until we implement CloseSession in this repository.