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

[CLIENT-3121] Support multi-record transactions #674

Merged
merged 288 commits into from
Nov 6, 2024
Merged

Conversation

juliannguyen4
Copy link
Collaborator

@juliannguyen4 juliannguyen4 commented Sep 17, 2024

Regression Testing

  • Massif memory usage looks ok
  • Build artifacts passes except for noise
  • 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
  • Check that new constants exist in tests... Don't need to test because type stubs test already checks that the module contains the constants
  • using c client code complete revision
  • using proper server config?
  • docs; explain what command and transaction mean
  • Dockerfile to deploy test server works with latest server release
  • Transaction class and MRT state constants could use better documentation (I am following the C client's docs)
  • Check code cov

@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2024

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 ⚠️
src/main/policy.c 70.83% 7 Missing ⚠️
src/main/transaction/type.c 91.42% 6 Missing ⚠️
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.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@juliannguyen4 juliannguyen4 marked this pull request as ready for review November 5, 2024 16:50
Copy link

@spinziyuval spinziyuval left a comment

Choose a reason for hiding this comment

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

Approved

Copy link
Contributor

@dwelch-spike dwelch-spike left a comment

Choose a reason for hiding this comment

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

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.

.gitmodules Show resolved Hide resolved
#include "types.h"
#include "client.h"

PyObject *AerospikeClient_Commit(AerospikeClient *self, PyObject *args,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with either method

src/main/client/mrt.c Show resolved Hide resolved
src/main/client/mrt.c Show resolved Hide resolved
@juliannguyen4
Copy link
Collaborator Author

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.

@dwelch-spike dwelch-spike self-requested a review November 6, 2024 15:33
Copy link
Contributor

@dwelch-spike dwelch-spike left a comment

Choose a reason for hiding this comment

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

LGTM

@juliannguyen4 juliannguyen4 merged commit d8eff6e into dev Nov 6, 2024
211 of 218 checks passed
@juliannguyen4 juliannguyen4 deleted the CLIENT-3121-mrt branch November 6, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants