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

Expose more SessionOptions from libbmq #7

Merged
merged 13 commits into from
Dec 6, 2023

Conversation

pniedzielski
Copy link
Collaborator

@pniedzielski pniedzielski commented Nov 29, 2023

This PR exposes all SessionOptions that have been added to libbmq since the creation of this Python SDK, except those that are deprecated or that configure distributed trace (which will require additional work on the Python end to enable). Please see commit messages for details.

The PR is broken into a series of patches for ease-of-review, but they should be squashed before merge, as each patch on its own will not pass the test suite.

Fixes #8.

@pniedzielski pniedzielski added the enhancement New feature or request label Nov 29, 2023
@pniedzielski pniedzielski self-assigned this Nov 29, 2023
@pniedzielski pniedzielski requested a review from a team as a code owner November 29, 2023 21:17
@pniedzielski
Copy link
Collaborator Author

Additionally: this PR contains the commits from #5, which are necessary to test these changes locally. That PR should be merged first.

@pniedzielski pniedzielski force-pushed the expose-session-options branch 2 times, most recently from eb31799 to 156efc0 Compare November 29, 2023 23:20
@quarter-note quarter-note assigned hallfox and unassigned pniedzielski Nov 30, 2023
src/blazingmq/_session.py Outdated Show resolved Hide resolved
src/blazingmq/_session.py Outdated Show resolved Hide resolved
tests/unit/test_session_options.py Outdated Show resolved Hide resolved
tests/unit/test_timeouts.py Outdated Show resolved Hide resolved
Since the creation of the BlazingMQ Python SDK, our underlying C++ SDK libbmq
has gained more `SessionOptions` that a user can configure, including
additional and more fine-grained timeout settings, new watermark
configurations, and buffer sizes.  In order to expose these new
`SessionOptions` to users of the Python SDK, we need to incnlude them in the
C++ library which our Cython layer calls, pybmq.

This patch exposes more of libbmq’s `SessionOptions` via the constructor of
pybmq’s `Session`.  After applying this patch, the pybmq should be able to
accept all `SessionOptions` from libbmq except those that are deprecated (e.g.,
`eventQueueSize`) and those that are related to distributed trace, which would
require more substantial integration work.  This work can be undertaken in the
future without impact on the changes here.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
The BlazingMQ Python SDK is divided into three layers, with the lowest level
being a pure C++ library pybmq that interfaces with the BlazingMQ C++ client
library libbmq, with the highest level being exposed through the `_session.py`
module’s `Session` Python class, and with a thin middle layer of Cython glue
code in `_ext.pyx` connecting the two.  This patch exposes the additional
`SessionOptions` through the Cython glue layer that the previous patch exposed
through pybmq’s `Session` class.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
Currently, the Python SDK does not allow users to specify different timeouts
for each session operation, instead accepting a single floating-point value
that sets open, configure, and close queue timeouts.  This is in contrast to
the C++ `libbmq` SDK, which allows users to configure timeouts for each
operation separately using the `bmqt_sessionoptions` component.

In order to preserve the behavior of existing clients, while enabling feature
parity between the Python SDK and the C++ SDK with regard to selecting
timeouts, this patch introduces a new value-semantic class, `Timeouts`, which
allows users to independently set the timeouts for each of the five session
operations whose timeouts are configureable: session connection, session
disconnection, queue opening, queue configuration, and queue closing.  It is
intended that clients may either continue to provide a `float` value with the
same semantics when creating a `Session` object, or may instead provide an
instance of `Timeouts` for the same parameter value to configure each timeout
separately.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
This patch completes the trio of patches that expose libbmq’s new
`SessionOptions` to the users of the Python SDK through the latter’s three
layers, by allowing users to build Python `Session`s that specify all the new
session options.

The changes in this patch should nonetheless maintain backwards compatibility
with all existing clients of the Python SDK.  All new session options that the
`Session` constructor accepts are defaulted to `None` and as such are not set
by pybmq, maintaining the existing behavior for callers who do not set any of
these new arguments.  New session options are also appended to the list of
arguments, so clients who build a `Session` using positional arguments rather
than named arguments will continue to function correctly.  Finally, by
overloading the `timeout` argument to taking either a simple float or an
instance of the new value-semantic class `Timeouts`, users who currently set a
timeout for the `Session` by passing in a single timeout will continue to see
the same behavior, but users passing in a new `Timeouts` value have access to
the more fine-grained timeout settings that libbmq’s `SessionOptions` provides.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
While users can continue to construct the `Session` directly using its
constructor, adding the additional session options arguments to the constructor
has made it somewhat unwieldly to call, especially if the user only wants to
configure one of the session options.  This patch provides an alternative,
simpler way to construct a `Session` instance, using a new value-semantic
builder class named `SessionOptions` that holds each of the new session
options and a class method named `with_options`, which takes an instance of
`SessionOptions` and returns a new `Session`.  `with_options` is the
recommended way to construct a new session if you plan to configure the session
options.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
The `MockSession` class in the Python SDK is used by our test suite to ensure
that session options are properly propagated through our Cython layer so they
can be passed to libbmq.  However, because have introduced new session options
into the Python SDK, the `MockSession` class needs to be updated so it knows
about these additional options.  This patch extends `MockSession` with the new
session options.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
Signed-off-by: Patrick M. Niedzielski <[email protected]>
This allows us to simplify the branching in `Session`’s constructor, and more
explicitly shows the existing behavior of passing a `float` rather than a
`Timeouts`.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
Copy link
Collaborator

@kaikulimu kaikulimu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments

src/blazingmq/_ext.pyx Outdated Show resolved Hide resolved
news/8.feature.rst Show resolved Hide resolved
src/blazingmq/_ext.pyi Outdated Show resolved Hide resolved
src/blazingmq/_session.py Show resolved Hide resolved
src/blazingmq/_session.py Show resolved Hide resolved
@pniedzielski pniedzielski force-pushed the expose-session-options branch from 15305c3 to 7999d4d Compare November 30, 2023 22:22
kaikulimu
kaikulimu previously approved these changes Nov 30, 2023
Copy link
Collaborator

@kaikulimu kaikulimu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/blazingmq/_ext.pyi Outdated Show resolved Hide resolved
src/blazingmq/_ext.pyx Outdated Show resolved Hide resolved
src/cpp/pybmq_mocksession.cpp Show resolved Hide resolved
@pniedzielski pniedzielski force-pushed the expose-session-options branch from da077b7 to 20d8f2b Compare December 6, 2023 17:18
@pniedzielski pniedzielski requested a review from 678098 December 6, 2023 19:37
@pniedzielski pniedzielski merged commit cdad1e9 into bloomberg:main Dec 6, 2023
10 checks passed
@pniedzielski pniedzielski deleted the expose-session-options branch December 6, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose more SessionOptions in the Python SDK
4 participants