-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: add python realization of ProduceOutput and RecordMetadata #393
feat: add python realization of ProduceOutput and RecordMetadata #393
Conversation
Added a comment in parent issue about maybe using PyO3 |
I am not sure that it would help here (but I may have a wrong understanding of its working. Using the But this is not really the intended effect, is it? It should be up to the user to decide whether they want to But my understanding may be wrong. |
Another option instead of using |
Using an option for the inner wrap would be a way to avoid the Cell and I think read a little clearer in the intent. |
5311d7f
to
64fecef
Compare
I updated the If the CI/CD goes through, the PR is ready from my perspective. I do have a failing test, though it is not mine and I am not sure if it is related: PS: I can squash the commits, if required. |
I think this looks good to me code wise. The CI all passed, so is the failing test on your system? |
It is a test failing locally on my installation, yes. To be honest, I don't quite understand, what that test is supposed to do. It connects to the cluster, does nothing and then expects there to be topics available (where should they come from?). |
refactor: use Option instead of Cell feat(test): add test for ProduceOutput and RecordMetadata feat: return ProduceOutput in async send
64fecef
to
97ce8a7
Compare
The commits are now squashed into one. |
I've run the tests on my system and they all pass. The test could fail if you don't have a currently operable fluvio profile that connects to a running cluster. The CI sets that up separately (and I have one by default too ). The simplest way is to make sure a local fluvio cluster is started first Merging the PR though, and thanks for the work and attention! |
Thank you for the merge. I do have a running cluster. I believe that the test fails if the original state of the fluvio cluster does not contain any topics (i.e. it is an empty cluster). Could you check, if your test also passes when you start the tests on a cluster with no topics existing in the beginning? |
Yes it seems to need a topic. and fails withoug. Seems like a temporary topic should be created as part of the test (as happens in many other tests already) Added an issue to remember to update this #397 |
Keeping an eye on maybe needing to revert this temporarily. Looks like CI runs are sometimes taking a long time as well as build errors that when integrated w/ a dev build PR (#396) |
This is a WIP pull request addressing issue #389.
It adds two python representations
ProduceOutput
andRecordMetadata
allowing to retrieve metadata for records sent by theTopicProducer
. The implementation tries to mimic the existing rust interface. However, the PyO3-wrapperProduceOutput
places the inner (native)ProduceOutput
into aCell
because its only methodmain
consumesself
, which can not be realized with a PyO3-wrapper.This means that a subsequent call to the
wait
method will find aDefault
version of the object and result in aProduceError::GetRecordMetadata(None)
(in pure Rust, such a subsequent call is impossible as the underlyingProduceOutput
would not exist anymore after the first call).I would like to hear your suggestions regarding this realization using
Cell
and if you have some good idea to avoid such a construction.