aerospike-client-python icon indicating copy to clipboard operation
aerospike-client-python copied to clipboard

[CLIENT-3121] Support multi-record transactions

Open juliannguyen4 opened this issue 1 year ago • 1 comments

Regression Testing

  • [x] Massif memory usage looks ok
  • [x] Build artifacts passes except for noise
  • [x] Valgrind shows no memory errors from this PR

Extra changes:

CI/CD

  • Always deploy enterprise edition server with strong consistency enabled along with security features
  • Create separate folder for Dockerfile build context when deploying an enterprise edition server
  • Allow running valgrind test on specific server version. This allows running against a server alpha version for MRTs when the latest tag for a server release candidate points to the next upcoming server release preceding MRTs
  • Add option to run build wheels with specific test files for quick smoke testing
  • Show exact test names that failed while tests are running during build wheels workflow
  • Fix documentation spell check workflow failing
  • Fix test node close listener for metrics
  • Set default Docker image name for Dockerfile that deploys an EE server for testing

Misc

  • Remove commented out list and map operations in type stubs that were removed in a prior release

Documentation changes: https://aerospike-python-client--674.org.readthedocs.build/en/674/client.html#multi-record-transactions https://aerospike-python-client--674.org.readthedocs.build/en/674/transaction.html https://aerospike-python-client--674.org.readthedocs.build/en/674/index.html

Extra changes: Cluster API breaking change: tran_count to command_count

todo

  • [ ] Transaction type stub needs improvement
  • [x] Check that new constants exist in tests... Don't need to test because type stubs test already checks that the module contains the constants
  • [x] using c client code complete revision
  • [x] using proper server config?
  • [x] docs; explain what command and transaction mean
  • [x] Dockerfile to deploy test server works with latest server release

juliannguyen4 avatar Sep 17 '24 16:09 juliannguyen4

Codecov Report

Attention: Patch coverage is 71.31783% with 37 lines in your changes missing coverage. Please review.

Project coverage is 80.79%. Comparing base (8ff0327) to head (666a3c6). Report is 8 commits behind head on dev.

Files with missing lines Patch % Lines
src/main/client/mrt.c 25.00% 24 Missing :warning:
src/main/policy.c 70.83% 7 Missing :warning:
src/main/transaction/type.c 91.42% 6 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #674      +/-   ##
==========================================
- Coverage   80.89%   80.79%   -0.10%     
==========================================
  Files         100      102       +2     
  Lines       15053    15173     +120     
==========================================
+ Hits        12177    12259      +82     
- Misses       2876     2914      +38     
Flag Coverage Δ
?

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Sep 17 '24 16:09 codecov-commenter

Looking good! I left a few nitpicks and have another thought. What do you think about making the mrt_commit_status_constants a class that subclasses enums.IntEnum? That way the user can tell what valid values are for those return codes through their IDE. This might not be easy to do and include in the base aerospike package since it is a C extension. If you have to define the class in C, I'd say it's not worth it. Let me know what you think.

Yeah it takes some effort to write an inheriting class in C; the C-API doesn't provide an API to create Enums. I was going to make a "dummy" package called aerospike and move the C-Python extension aerospike into that package, so we can declare types in Python using __init__.py, but I wasn't able to get it to work.

juliannguyen4 avatar Nov 06 '24 15:11 juliannguyen4