-
Notifications
You must be signed in to change notification settings - Fork 111
[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 13 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 |
|---|---|---|
|
|
@@ -329,6 +329,14 @@ Other | |
| digest = aerospike.calc_digest("test", "demo", 1 ) | ||
| pp.pprint(digest) | ||
|
|
||
| .. py:function:: get_partition_id(digest) -> int | ||
|
|
||
| Calculate the partition id for a record using its digest. | ||
|
|
||
| :param bytes-like object digest: the record's digest calculated by :py:meth:`aerospike.calc_digest`. | ||
|
||
| :return: the record's partition id | ||
| :rtype: :class:`int` | ||
|
|
||
| .. _client_config: | ||
|
|
||
| Client Configuration | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,20 +131,40 @@ PyObject *Aerospike_Calc_Digest(PyObject *self, PyObject *args, PyObject *kwds) | |
| return Aerospike_Calc_Digest_Invoke(py_ns, py_set, py_key); | ||
| } | ||
|
|
||
| PyObject *Aerospike_Get_Partition_Id(PyObject *self, PyObject *args) | ||
| PyObject *Aerospike_Get_Partition_Id(PyObject *self, PyObject *arg) | ||
| { | ||
| // Python Function Arguments | ||
| as_digest_value digest; | ||
| Py_buffer py_buffer; | ||
| PyObject *py_retval = NULL; | ||
|
|
||
| // Python Function Argument Parsing | ||
| if (PyArg_Parse(args, "(s)", &digest) == false) { | ||
| return NULL; | ||
| if (PyArg_Parse(arg, "y*", &py_buffer) == false) { | ||
|
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. 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. ` ` Also requires changing
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 think switching to y# documentation: https://docs.python.org/3/c-api/arg.html#:~:text=accept%20binary%20data.-,y#,-(read%2Donly%20bytes
To give more context, this PR is for We could make a breaking change to |
||
| goto exit; | ||
| } | ||
|
|
||
| uint32_t part_id = 0; | ||
| as_error err; | ||
| as_error_init(&err); | ||
|
|
||
| if (py_buffer.len != 20) { | ||
| as_error_update(&err, AEROSPIKE_ERR_PARAM, | ||
| "Digest must be 20 bytes long"); | ||
| goto CLEANUP_AND_EXIT; | ||
| } | ||
|
|
||
| part_id = as_partition_getid(digest, 4096); | ||
| uint32_t part_id = as_partition_getid(py_buffer.buf, 4096); | ||
|
|
||
| // Invoke Operation | ||
| return PyLong_FromLong(part_id); | ||
| py_retval = PyLong_FromLong(part_id); | ||
| if (!py_retval) { | ||
| as_error_update(&err, AEROSPIKE_ERR_CLIENT, | ||
| "Failed to retrieve partition id"); | ||
| goto CLEANUP_AND_EXIT; | ||
| } | ||
|
|
||
| CLEANUP_AND_EXIT: | ||
| PyBuffer_Release(&py_buffer); | ||
|
|
||
| if (err.code != AEROSPIKE_OK) { | ||
| raise_exception(&err); | ||
| } | ||
|
|
||
| exit: | ||
| return py_retval; | ||
| } | ||
| 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.
Isn't the partition_id associated with key's digest? It doesn't have to be a record right?
Also Nit: Capital ID looks better in documentation than id.
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.
That's a good point, will fix