-
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 11 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 |
|---|---|---|
|
|
@@ -933,8 +933,15 @@ async def main(): | |
| print(f"Error with partitioned KV table: {e}") | ||
| traceback.print_exc() | ||
|
|
||
|
|
||
| print("\n--- New: async context manager demo ---") | ||
| async with await fluss.FlussConnection.create(config) as demo_conn: | ||
| demo_table = await demo_conn.get_table(table_path) | ||
| async with demo_table.new_append().create_writer() as writer: | ||
| writer.append({"id": 1, "name": "demo", "score": 1.0}) | ||
| # auto-flushes on exit | ||
| # Close connection | ||
| conn.close() | ||
|
Contributor
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. This seems to be a breaking change, we should mention it. Documentation should be updated as well where appropriate.
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 @leekeiabstraction, updated the docs in 683bcbc.
Contributor
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. @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). |
||
| await conn.close() | ||
| print("\nConnection closed") | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -253,6 +253,13 @@ class FlussConnection: | |
| exc_value: Optional[BaseException], | ||
| traceback: Optional[TracebackType], | ||
| ) -> bool: ... | ||
| async def __aenter__(self) -> FlussConnection: ... | ||
| async def __aexit__( | ||
| self, | ||
| exc_type: Optional[type], | ||
| exc_value: Optional[BaseException], | ||
| traceback: Optional[TracebackType], | ||
| ) -> bool: ... | ||
| def __repr__(self) -> str: ... | ||
|
|
||
| class ServerNode: | ||
|
|
@@ -611,6 +618,37 @@ class AppendWriter: | |
| def write_arrow_batch(self, batch: pa.RecordBatch) -> WriteResultHandle: ... | ||
| def write_pandas(self, df: pd.DataFrame) -> None: ... | ||
| async def flush(self) -> None: ... | ||
| async def __aenter__(self) -> AppendWriter: | ||
| """ | ||
| Enter the async context manager. | ||
|
|
||
| Returns: | ||
| The AppendWriter instance. | ||
| """ | ||
| ... | ||
| async def __aexit__( | ||
| self, | ||
| exc_type: Optional[type], | ||
| exc_value: Optional[BaseException], | ||
| traceback: Optional[TracebackType], | ||
| ) -> bool: | ||
| """ | ||
| Exit the async context manager. | ||
|
|
||
| On successful exit, the writer is automatically flushed to ensure | ||
| all pending records are sent and acknowledged. | ||
|
|
||
| Note on Exceptions: | ||
| If an exception occurs inside the `async with` block, `flush()` is | ||
| bypassed to return control to the event loop immediately. However, | ||
| any records already passed to `append()` prior to the exception | ||
| reside in a shared background buffer and will still be transmitted | ||
| to the server. | ||
|
|
||
| To achieve true atomicity, buffer your records in a Python list and | ||
| write them in a single batch at the end of your logic. | ||
| """ | ||
| ... | ||
| def __repr__(self) -> str: ... | ||
|
|
||
| class UpsertWriter: | ||
|
|
@@ -644,6 +682,37 @@ class UpsertWriter: | |
| async def flush(self) -> None: | ||
| """Flush all pending upsert/delete operations to the server.""" | ||
| ... | ||
| async def __aenter__(self) -> UpsertWriter: | ||
| """ | ||
| Enter the async context manager. | ||
|
|
||
| Returns: | ||
| The UpsertWriter instance. | ||
| """ | ||
| ... | ||
| async def __aexit__( | ||
| self, | ||
| exc_type: Optional[type], | ||
| exc_value: Optional[BaseException], | ||
| traceback: Optional[TracebackType], | ||
| ) -> bool: | ||
| """ | ||
| Exit the async context manager. | ||
|
|
||
| On successful exit, the writer is automatically flushed to ensure | ||
| all pending records are sent and acknowledged. | ||
|
|
||
| Note on Exceptions: | ||
| If an exception occurs inside the `async with` block, `flush()` is | ||
| bypassed to return control to the event loop immediately. However, | ||
| any records already passed to `upsert()` or `delete()` prior to the | ||
| exception reside in a shared background buffer and will still be | ||
| transmitted to the server. | ||
|
|
||
| To achieve true atomicity, buffer your records in a Python list and | ||
| write them in a single batch at the end of your logic. | ||
| """ | ||
| ... | ||
| def __repr__(self) -> str: ... | ||
|
|
||
|
|
||
|
|
@@ -807,6 +876,15 @@ class LogScanner: | |
|
|
||
| You must call subscribe(), subscribe_buckets(), or subscribe_partition() first. | ||
| """ | ||
| ... | ||
| def close(self) -> None: ... | ||
|
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 think these guys were deleted
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 8bec2d8. |
||
| async def __aenter__(self) -> LogScanner: ... | ||
|
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. why have we deleted close but left the rest?
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, good catch, removed in 195579f. |
||
| async def __aexit__( | ||
| self, | ||
| exc_type: Optional[type], | ||
| exc_value: Optional[BaseException], | ||
| traceback: Optional[TracebackType], | ||
| ) -> bool: ... | ||
| def __repr__(self) -> str: ... | ||
| def __aiter__(self) -> AsyncIterator[Union[ScanRecord, RecordBatch]]: ... | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,9 @@ | |
| use crate::*; | ||
| use pyo3_async_runtimes::tokio::future_into_py; | ||
| use std::sync::Arc; | ||
| use std::time::Duration; | ||
|
|
||
| const DEFAULT_CLOSE_TIMEOUT_SECS: u64 = 30; | ||
|
|
||
| /// Connection to a Fluss cluster | ||
| #[pyclass] | ||
|
|
@@ -82,9 +85,27 @@ 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. | ||
| /// | ||
| /// Args: | ||
| /// timeout_ms: The timeout in milliseconds to wait for the graceful drain. | ||
| /// If not provided, defaults to 30 seconds. | ||
| #[pyo3(signature = (timeout_ms=None))] | ||
|
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. we can't pass params to async with, so let's leave just default timeout or we can define config if and let users configure it statically if ever needed. Also Java uses Long.MAX_VALUE, so let's match with Duration::MAX
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 8bec2d8. |
||
| fn close<'py>(&self, py: Python<'py>, timeout_ms: Option<u64>) -> PyResult<Bound<'py, PyAny>> { | ||
| let inner = self.inner.clone(); | ||
| let timeout = timeout_ms | ||
| .map(Duration::from_millis) | ||
| .unwrap_or_else(|| Duration::from_secs(DEFAULT_CLOSE_TIMEOUT_SECS)); | ||
|
|
||
| future_into_py(py, async move { | ||
| inner | ||
| .close(timeout) | ||
| .await | ||
| .map_err(|e| FlussError::from_core_error(&e)) | ||
| }) | ||
| } | ||
|
|
||
| // Enter the runtime context (for 'with' statement) | ||
|
|
@@ -100,10 +121,36 @@ 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(); | ||
| future_into_py(py, async move { | ||
| inner | ||
| .close(Duration::from_secs(DEFAULT_CLOSE_TIMEOUT_SECS)) | ||
| .await | ||
| .map_err(|e| 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,37 @@ 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 successful exit, the writer is automatically flushed. | ||
| /// If an exception occurs, the flush is skipped to allow immediate error | ||
| /// propagation, though pending records may still be sent in the background. | ||
| #[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 has_error = exc_type.is_some(); | ||
| let inner = self.inner.clone(); | ||
| future_into_py(py, async move { | ||
| if !has_error { | ||
| inner | ||
| .flush() | ||
| .await | ||
| .map_err(|e| 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,37 @@ 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 successful exit, the writer is automatically flushed. | ||
| /// If an exception occurs, the flush is skipped to allow immediate error | ||
| /// propagation, though pending records may still be sent in the background. | ||
| #[pyo3(signature = (exc_type=None, _exc_value=None, _traceback=None))] | ||
|
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. ditto
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. |
||
| 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 has_error = exc_type.is_some(); | ||
| let writer = self.writer.clone(); | ||
| future_into_py(py, async move { | ||
| if !has_error { | ||
| writer | ||
| .flush() | ||
| .await | ||
| .map_err(|e| 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.
wouldn't it crash in runtime? we have more columns in schema.
Can you make sure example.py works e2e?
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 @fresh-borzoni, thanks for catching this. Updated the missing columns in 54a0361, then rearranged the async context manager test in ca0a4ad. The
example.pyis now working e2e from my local setup.