-
Notifications
You must be signed in to change notification settings - Fork 112
[CLIENT-4086] (Breaking change) Have aerospike.get_partition_id() take in a bytearray as argument instead of a tuple containing a str. Add missing documentation for aerospike.get_partition_id(). #928
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
dbee744
ff34ff6
a125dae
d2fdf8a
e4a7a8c
bf66663
de3b6b7
25182fb
dd26357
3799fbe
69ceeac
e746916
743b4bb
b8572ad
947a9ce
53aa749
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 |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| from aerospike import exception as e | ||
| import aerospike | ||
| import pytest | ||
|
|
||
| # This isn't a correctness test. It's only for code coverage purposes | ||
| # and to make sure the API is aligned with the documentation | ||
|
Collaborator
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. Will QE be testing the correctness of this? Should be right, but we should confirm with testing. Should be easy to verify with AS_POLICY_KEY_DIGEST.
Collaborator
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. I'll just manually check using gdb. I don't think it's necessary to write a correctness test since customers shouldn't be using this (it's only meant for internal testing)
Collaborator
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. I'm starting to see that maybe this API call isn't necessary.. we can just use gdb to print the partition id... |
||
| class TestGetPartitionID: | ||
| def test_basic_usage(self): | ||
| digest = aerospike.calc_digest("test", "demo", 1) | ||
| part_id = aerospike.get_partition_id(digest) | ||
| assert type(part_id) == int | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "digest, expected_exception", | ||
| [ | ||
| # Digests must be exactly 20 bytes long | ||
| (bytearray([0] * 21), e.ParamError), | ||
| (bytearray([0] * 19), e.ParamError), | ||
| # Does not accept strings | ||
| ("1" * 20, TypeError) | ||
| ] | ||
| ) | ||
| def test_invalid_digest(self, digest, expected_exception): | ||
| with pytest.raises(expected_exception): | ||
| aerospike.get_partition_id(digest) | ||
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.
To optimize, you should change to "y#" in order to avoid creating an entire py_buffer object:
Since you don't need to write to the buffer, using read only mode should be faster.
Snippet:
`
const uint8_t *buf;
Py_ssize_t len;
`
Also requires changing
METH_Oback toMETH_VARARGS.Uh oh!
There was an error while loading. Please reload this page.
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.
I think switching to
"y#"would makeget_partition_id()andcalc_digest()harder to use. The user can no longer pass a digest fromaerospike.calc_digest()directly intoaerospike.get_partition_id().calc_digest()returns a bytearray which is mutable, and "y#" only accepts a "read-only bytes-like object"y# documentation: https://docs.python.org/3/c-api/arg.html#:~:text=accept%20binary%20data.-,y#,-(read%2Donly%20bytes
aerospike.calc_digest()has returned a bytearray for a while now: https://aerospike-python-client.readthedocs.io/en/8.0.0/aerospike.html#aerospike.calc_digestTo give more context, this PR is for
aerospike.get_partition_id()debugging QE's bank test. (the Jira ticket explains more about whataerospike.get_partition_id()is used for) I don't believe customers are using this in production.We could make a breaking change to
aerospike.calc_digest()to return abytesobject instead ofbytearray;bytesis immutable whereas the latter is mutable. But I feel like that's outside the scope of this PR