-
Notifications
You must be signed in to change notification settings - Fork 49
feat: (Python) Add async context manager #487
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
Changes from all commits
2a29c63
dec0921
180ad11
a1cadf2
ff10371
a6a5bff
df9e8c7
713e5c2
0fa4500
efbf4f7
c1546fc
5e0adc8
8bec2d8
195579f
54a0361
f634652
ca0a4ad
97687a8
96a9c7a
683bcbc
e110f96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| use crate::*; | ||
| use pyo3_async_runtimes::tokio::future_into_py; | ||
| use std::sync::Arc; | ||
| use std::time::Duration; | ||
|
|
||
| /// Connection to a Fluss cluster | ||
| #[pyclass] | ||
|
|
@@ -82,9 +83,19 @@ impl FlussConnection { | |
| }) | ||
| } | ||
|
|
||
| // Close the connection | ||
| fn close(&mut self) -> PyResult<()> { | ||
| Ok(()) | ||
| /// Close the connection (async). | ||
| /// | ||
| /// Gracefully shuts down the connection by draining any pending write batches. | ||
| /// This method is awaitable. | ||
| fn close<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> { | ||
| let inner = self.inner.clone(); | ||
|
|
||
| future_into_py(py, async move { | ||
| inner | ||
| .close(Duration::MAX) | ||
| .await | ||
| .map_err(|e| FlussError::from_core_error(&e)) | ||
| }) | ||
| } | ||
|
|
||
| // Enter the runtime context (for 'with' statement) | ||
|
|
@@ -100,10 +111,39 @@ impl FlussConnection { | |
| _exc_value: Option<Bound<'_, PyAny>>, | ||
| _traceback: Option<Bound<'_, PyAny>>, | ||
| ) -> PyResult<bool> { | ||
| self.close()?; | ||
| // Sync exit cannot await the graceful drain, so it's a no-op here. | ||
| // Users should use 'async with' for graceful shutdown. | ||
| Ok(false) | ||
| } | ||
|
|
||
| // Enter the async runtime context (for 'async with' statement) | ||
| fn __aenter__<'py>(slf: PyRef<'py, Self>, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> { | ||
| let py_slf = slf.into_pyobject(py)?.unbind(); | ||
| future_into_py(py, async move { Ok(py_slf) }) | ||
| } | ||
|
|
||
| // Exit the async runtime context (for 'async with' statement) | ||
| #[pyo3(signature = (exc_type=None, _exc_value=None, _traceback=None))] | ||
| fn __aexit__<'py>( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: mirror the writers' is_exc_none guard
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @fresh-borzoni, thanks for catching this, fixed in f634652. |
||
| &self, | ||
| py: Python<'py>, | ||
| exc_type: Option<Bound<'py, PyAny>>, | ||
| _exc_value: Option<Bound<'py, PyAny>>, | ||
| _traceback: Option<Bound<'py, PyAny>>, | ||
| ) -> PyResult<Bound<'py, PyAny>> { | ||
| let inner = self.inner.clone(); | ||
| let is_exc_none = exc_type.as_ref().is_none_or(|e| e.is_none()); | ||
| future_into_py(py, async move { | ||
| let res = inner.close(Duration::MAX).await; | ||
| if let Err(e) = res { | ||
| if is_exc_none { | ||
| return Err(FlussError::from_core_error(&e)); | ||
| } | ||
| } | ||
| Ok(false) | ||
| }) | ||
| } | ||
|
Comment on lines
+125
to
+145
|
||
|
|
||
| fn __repr__(&self) -> String { | ||
| "FlussConnection()".to_string() | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -989,6 +989,35 @@ impl AppendWriter { | |
| }) | ||
| } | ||
|
|
||
| // Enter the async runtime context (for 'async with' statement) | ||
| fn __aenter__<'py>(slf: PyRef<'py, Self>, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> { | ||
| let py_slf = slf.into_pyobject(py)?.unbind(); | ||
| future_into_py(py, async move { Ok(py_slf) }) | ||
| } | ||
|
|
||
| // Exit the async runtime context (for 'async with' statement) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need this logic. The whole idea of context managers is guaranteed cleanup, and here we skip flush() just to return the error faster - that doesn't match. Can we just always call flush() on exit
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @fresh-borzoni, this has been addressed in 5e0adc8. |
||
| /// On exit, the writer is automatically flushed. | ||
| #[pyo3(signature = (exc_type=None, _exc_value=None, _traceback=None))] | ||
| fn __aexit__<'py>( | ||
| &self, | ||
| py: Python<'py>, | ||
| exc_type: Option<Bound<'py, PyAny>>, | ||
| _exc_value: Option<Bound<'py, PyAny>>, | ||
| _traceback: Option<Bound<'py, PyAny>>, | ||
| ) -> PyResult<Bound<'py, PyAny>> { | ||
| let inner = self.inner.clone(); | ||
| let is_exc_none = exc_type.as_ref().is_none_or(|e| e.is_none()); | ||
| future_into_py(py, async move { | ||
| let res = inner.flush().await; | ||
| if let Err(e) = res { | ||
| if is_exc_none { | ||
| return Err(FlussError::from_core_error(&e)); | ||
| } | ||
| } | ||
| Ok(false) | ||
| }) | ||
| } | ||
|
Comment on lines
+998
to
+1019
|
||
|
|
||
| fn __repr__(&self) -> String { | ||
| "AppendWriter()".to_string() | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,6 +108,35 @@ impl UpsertWriter { | |
| }) | ||
| } | ||
|
|
||
| // Enter the async runtime context (for 'async with' statement) | ||
| fn __aenter__<'py>(slf: PyRef<'py, Self>, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> { | ||
| let py_slf = slf.into_pyobject(py)?.unbind(); | ||
| future_into_py(py, async move { Ok(py_slf) }) | ||
| } | ||
|
|
||
| // Exit the async runtime context (for 'async with' statement) | ||
| /// On exit, the writer is automatically flushed. | ||
| #[pyo3(signature = (exc_type=None, _exc_value=None, _traceback=None))] | ||
| fn __aexit__<'py>( | ||
| &self, | ||
| py: Python<'py>, | ||
| exc_type: Option<Bound<'py, PyAny>>, | ||
| _exc_value: Option<Bound<'py, PyAny>>, | ||
| _traceback: Option<Bound<'py, PyAny>>, | ||
| ) -> PyResult<Bound<'py, PyAny>> { | ||
| let writer = self.writer.clone(); | ||
| let is_exc_none = exc_type.as_ref().is_none_or(|e| e.is_none()); | ||
| future_into_py(py, async move { | ||
| let res = writer.flush().await; | ||
| if let Err(e) = res { | ||
| if is_exc_none { | ||
| return Err(FlussError::from_core_error(&e)); | ||
| } | ||
| } | ||
| Ok(false) | ||
| }) | ||
|
Comment on lines
+117
to
+137
|
||
| } | ||
|
|
||
| fn __repr__(&self) -> String { | ||
| "UpsertWriter()".to_string() | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a breaking change, we should mention it. Documentation should be updated as well where appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @leekeiabstraction, updated the docs in 683bcbc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fresh-borzoni wdyt about this?
I think as the next release will probably be 1.0, having breaking change is fine.
Unrelated but outside of this PR, we probably should think about places that might benefit from some breaking changes as well eg properly enforcing pub (crate).