Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for closing a database (session) #13

Open
kou opened this issue Feb 9, 2023 · 15 comments
Open

Add support for closing a database (session) #13

kou opened this issue Feb 9, 2023 · 15 comments

Comments

@kou
Copy link
Member

kou commented Feb 9, 2023

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
Copy link
Member

lidavidm commented Feb 9, 2023

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
Copy link
Member

lidavidm commented Feb 9, 2023

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

Would you like to follow up there?

@kou
Copy link
Member Author

kou commented Feb 10, 2023

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

@kou
Copy link
Member Author

kou commented Apr 12, 2023

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

@zhjwpku
Copy link

zhjwpku commented Sep 16, 2023

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
Copy link
Member Author

kou commented Sep 19, 2023

No there isn't.
We need to forward the discussion referred at #13 (comment) for it. Could you join the discussion on the dev@arrow.apache.org mailing list?

@kou
Copy link
Member Author

kou commented Oct 25, 2023

Ah, apache/arrow#34817 is a proposed implementation.
I thought that it's not related to #13 (comment) discussion.

@cisaacson
Copy link

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
Copy link
Member Author

kou commented Oct 5, 2024

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

@cisaacson
Copy link

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

  • Is this implemented for arrow-rs? I do not believe so as CloseSession does not appear in the codebase.
  • Is there an example you can point me to?

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

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
Copy link
Member Author

kou commented Oct 6, 2024

We need the followings for explicit close:

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

@cisaacson
Copy link

@kou Sure, I will report it there.

@cisaacson
Copy link

cisaacson commented Oct 6, 2024

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

@kou
Copy link
Member Author

kou commented Oct 7, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants